From f30685d9c235c8a8a019da0fa1abdbf835494db9 Mon Sep 17 00:00:00 2001 From: Bogdan Bodnar Date: Wed, 4 Dec 2019 17:05:56 +0100 Subject: [PATCH 1/3] Add WS unsubscribtion. Code review improvements. --- src/webSockets/WebSockets.js | 41 ++++++++++++++++++++++++++++++------ src/webSockets/hooks.js | 19 ++++++++++++----- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/webSockets/WebSockets.js b/src/webSockets/WebSockets.js index 6fb9c50..c01407f 100644 --- a/src/webSockets/WebSockets.js +++ b/src/webSockets/WebSockets.js @@ -61,9 +61,36 @@ export class WebSockets { return this; } - subscribe(params) { + subscribe(module) { this.waitForConnection(() => { - this.send("subscribe", params); + this.send("subscribe", module); + }); + return this; + } + + unbind(module, action, callback) { + const callbacks = this.callbacks[module][action]; + + const index = callbacks.indexOf(callback); + if (index !== -1) { + callbacks.splice(index, 1); + } + + if (callbacks.length === 0) { + delete this.callbacks[module][action]; + } + + if (Object.keys(this.callbacks[module]).length === 0) { + this.unsubscribe(module); + } + + return this; + } + + unsubscribe(module) { + this.waitForConnection(() => { + this.send("unsubscribe", module); + delete this.callbacks[module]; }); return this; } @@ -82,15 +109,15 @@ export class WebSockets { let chain; try { chain = this.callbacks[json.module][json.action]; - } catch (e) { - if (e instanceof TypeError) { - console.log(`Callback for this message wasn't found:${e.data}`); - } else throw e; + } catch (error) { + if (error instanceof TypeError) { + console.log(`Callback for this message wasn't found:${error.data}`); + } else throw error; } if (typeof chain === "undefined") return; - for (let i = 0; i < chain.length; i++) chain[i](json); + chain.forEach((callback) => callback(json)); } close() { diff --git a/src/webSockets/hooks.js b/src/webSockets/hooks.js index 3ca0084..7c0fc61 100644 --- a/src/webSockets/hooks.js +++ b/src/webSockets/hooks.js @@ -11,12 +11,21 @@ export function useWSForisModule(ws, module, action = "update_settings") { const [data, setData] = useState(null); useEffect(() => { - if (ws && module) { - ws.subscribe(module) - .bind(module, action, (msg) => { - setData(msg.data); - }); + // Sometime we want to disable this hook if WS is not passed. We can't make conditional + // hooks, but we can disable it here. It's used especially in ForisForm when a module + // doesn't present any WS endpoint. + if (!ws) return; + + function callback(msg) { + setData(msg.data); } + + ws.subscribe(module) + .bind(module, action, callback); + + return () => { + ws.unbind(module, action, callback); + }; }, [action, module, ws]); return [data]; From 9d322811c330a78a6bbf14f889b5aff80ab830a7 Mon Sep 17 00:00:00 2001 From: Bogdan Bodnar Date: Fri, 6 Dec 2019 12:07:42 +0100 Subject: [PATCH 2/3] Improve docs and propTypes of WS usage in ForisForm. --- src/form/components/ForisForm.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/form/components/ForisForm.js b/src/form/components/ForisForm.js index 960ffd6..80e3821 100644 --- a/src/form/components/ForisForm.js +++ b/src/form/components/ForisForm.js @@ -20,14 +20,16 @@ import { useForisModule, useForm } from "../hooks"; import { STATES as SUBMIT_BUTTON_STATES, SubmitButton } from "./SubmitButton"; ForisForm.propTypes = { - /** WebSocket object see `scr/common/WebSockets.js`. */ + /** Optional WebSocket object. See `scr/common/WebSockets.js`. + * `forisConfig.wsModule` should be specified when it's passed. + * */ ws: PropTypes.object, /** Foris configuration object. See usage in main components. */ forisConfig: PropTypes.shape({ /** reForis Flask aplication API endpoint from `src/common/API.js`. */ endpoint: PropTypes.string.isRequired, /** `foris-controller` module name to be used via WebSockets. - * If it's not passed then WebSockets aren't used + * It can be use only with `ws` prop. * */ wsModule: PropTypes.string, /** `foris-controller` action name to be used via WebSockets. @@ -49,6 +51,17 @@ ForisForm.propTypes = { children: PropTypes.node.isRequired, /** Optional override of form submit callback */ onSubmitOverridden: PropTypes.func, + + // eslint-disable-next-line react/no-unused-prop-types + customWSProp(props) { + const wsModuleIsSpecified = !!(props.forisConfig && props.forisConfig.wsModule); + if (props.ws && !wsModuleIsSpecified) { + return new Error("forisConfig.wsModule should be specified when ws object is passed."); + } + if (!props.ws && wsModuleIsSpecified) { + return new Error("forisConfig.wsModule is specified without passing ws object."); + } + }, }; ForisForm.defaultProps = { From d55615abcc2340fe4e5f8e421814a42ff9e3d4d7 Mon Sep 17 00:00:00 2001 From: Bogdan Bodnar Date: Fri, 6 Dec 2019 12:09:54 +0100 Subject: [PATCH 3/3] Grammar. --- src/webSockets/hooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webSockets/hooks.js b/src/webSockets/hooks.js index 7c0fc61..7357ead 100644 --- a/src/webSockets/hooks.js +++ b/src/webSockets/hooks.js @@ -11,7 +11,7 @@ export function useWSForisModule(ws, module, action = "update_settings") { const [data, setData] = useState(null); useEffect(() => { - // Sometime we want to disable this hook if WS is not passed. We can't make conditional + // Sometimes we want to disable this hook if WS is not passed. We can't make conditional // hooks, but we can disable it here. It's used especially in ForisForm when a module // doesn't present any WS endpoint. if (!ws) return;