From 6cabcc611fc40a3b4c3eef134e63b1ebe803115e Mon Sep 17 00:00:00 2001 From: rmorshea Date: Fri, 27 Aug 2021 01:36:43 -0700 Subject: [PATCH 1/2] fix model mutation in client The client-side model was being mutated on each update in order to try and be more performant. This is a problem though because React relies on checking the identity of some parts of the model to see if things have changed. No we create a copy of the model each time it changed --- src/client/package-lock.json | 2 +- .../packages/idom-client-react/package.json | 2 +- .../idom-client-react/src/component.js | 34 +-------------- .../packages/idom-client-react/src/utils.js | 41 ++++++++++++++----- tests/test_client.py | 39 ++++++++++++++++++ 5 files changed, 73 insertions(+), 45 deletions(-) diff --git a/src/client/package-lock.json b/src/client/package-lock.json index 3f82c9dc3..dadf81544 100644 --- a/src/client/package-lock.json +++ b/src/client/package-lock.json @@ -2661,7 +2661,7 @@ }, "packages/idom-app-react/packages/idom-client-react": {}, "packages/idom-client-react": { - "version": "0.9.0", + "version": "0.9.2", "license": "MIT", "dependencies": { "fast-json-patch": "^3.0.0-1", diff --git a/src/client/packages/idom-client-react/package.json b/src/client/packages/idom-client-react/package.json index 4af848aca..ed86c10b1 100644 --- a/src/client/packages/idom-client-react/package.json +++ b/src/client/packages/idom-client-react/package.json @@ -1,7 +1,7 @@ { "name": "idom-client-react", "description": "A client for IDOM implemented in React", - "version": "0.9.1", + "version": "0.9.2", "author": "Ryan Morshead", "license": "MIT", "type": "module", diff --git a/src/client/packages/idom-client-react/src/component.js b/src/client/packages/idom-client-react/src/component.js index ad5d30359..ffc0712bb 100644 --- a/src/client/packages/idom-client-react/src/component.js +++ b/src/client/packages/idom-client-react/src/component.js @@ -4,7 +4,7 @@ import htm from "htm"; import serializeEvent from "./event-to-object.js"; -import { applyPatchInplace, joinUrl } from "./utils.js"; +import { useJsonPatchCallback } from "./utils.js"; const html = htm.bind(React.createElement); export const LayoutConfigContext = React.createContext({ @@ -13,7 +13,7 @@ export const LayoutConfigContext = React.createContext({ }); export function Layout({ saveUpdateHook, sendEvent, loadImportSource }) { - const [model, patchModel] = useInplaceJsonPatch({}); + const [model, patchModel] = useJsonPatchCallback({}); React.useEffect(() => saveUpdateHook(patchModel), [patchModel]); @@ -181,33 +181,3 @@ function loadImportSource(config, importSource) { } }); } - -function useInplaceJsonPatch(doc) { - const ref = React.useRef(doc); - const forceUpdate = useForceUpdate(); - - const applyPatch = React.useCallback( - (path, patch) => { - applyPatchInplace(ref.current, path, patch); - forceUpdate(); - }, - [ref, forceUpdate] - ); - - return [ref.current, applyPatch]; -} - -function useForceUpdate() { - const [, updateState] = React.useState(); - return React.useCallback(() => updateState({}), []); -} - -function useConst(func) { - const ref = React.useRef(); - - if (!ref.current) { - ref.current = func(); - } - - return ref.current; -} diff --git a/src/client/packages/idom-client-react/src/utils.js b/src/client/packages/idom-client-react/src/utils.js index 85358ab00..09a4200c2 100644 --- a/src/client/packages/idom-client-react/src/utils.js +++ b/src/client/packages/idom-client-react/src/utils.js @@ -1,16 +1,35 @@ +import React from "react"; import jsonpatch from "fast-json-patch"; -export function applyPatchInplace(doc, pathPrefix, patch) { - if (pathPrefix) { - patch = patch.map((op) => - Object.assign({}, op, { path: pathPrefix + op.path }) - ); - } - jsonpatch.applyPatch(doc, patch, false, true); +export function useJsonPatchCallback(initial) { + const model = React.useRef(initial); + const forceUpdate = useForceUpdate(); + + const applyPatch = React.useCallback( + (pathPrefix, patch) => { + if (pathPrefix) { + patch = patch.map((op) => + Object.assign({}, op, { path: pathPrefix + op.path }) + ); + } + // Always return a newDocument because React checks some attributes of the model + // (e.g. model.attributes.style is checked for identity) + model.current = jsonpatch.applyPatch( + model.current, + patch, + false, + false, + true + ).newDocument; + forceUpdate(); + }, + [model] + ); + + return [model.current, applyPatch]; } -export function joinUrl(base, tail) { - return tail.startsWith("./") - ? (base.endsWith("/") ? base.slice(0, -1) : base) + tail.slice(1) - : tail; +function useForceUpdate() { + const [, updateState] = React.useState(); + return React.useCallback(() => updateState({}), []); } diff --git a/tests/test_client.py b/tests/test_client.py index 9fbf50b74..b2dc43708 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -43,3 +43,42 @@ def NewComponent(): # check that we can resume normal operation set_state.current(1) driver.find_element_by_id("new-component-1") + + +def test_style_can_be_changed(display, driver, driver_wait): + """This test was introduced to verify the client does not mutate the model + + A bug was introduced where the client-side model was mutated and React was relying + on the model to have been copied in order to determine if something had changed. + + See for more info: https://github.com/idom-team/idom/issues/480 + """ + + @idom.component + def ButtonWithChangingColor(): + color_toggle, set_color_toggle = idom.hooks.use_state(True) + color = "red" if color_toggle else "blue" + return idom.html.button( + { + "id": "my-button", + "onClick": lambda event: set_color_toggle(not color_toggle), + "style": {"backgroundColor": color, "color": "white"}, + }, + f"color: {color}", + ) + + display(ButtonWithChangingColor) + + button = driver.find_element_by_id("my-button") + + assert get_style(button)["background-color"] == "red" + + for color in ["blue", "red"] * 2: + button.click() + driver_wait.until(lambda _: get_style(button)["background-color"] == color) + + +def get_style(element): + items = element.get_attribute("style").split(";") + pairs = [item.split(":", 1) for item in map(str.strip, items) if item] + return {key.strip(): value.strip() for key, value in pairs} From dca2b06440e0421918ee6a11c4b9f6936fe44ac8 Mon Sep 17 00:00:00 2001 From: rmorshea Date: Fri, 27 Aug 2021 01:37:31 -0700 Subject: [PATCH 2/2] fix snake game --- docs/source/_static/custom.js | 418 ++++++++++++++--------------- docs/source/examples/snake_game.py | 6 +- 2 files changed, 212 insertions(+), 212 deletions(-) diff --git a/docs/source/_static/custom.js b/docs/source/_static/custom.js index 735653ac7..6b5c941fd 100644 --- a/docs/source/_static/custom.js +++ b/docs/source/_static/custom.js @@ -620,6 +620,187 @@ function checkDCE() { var n=function(t,s,r,e){var u;s[0]=0;for(var h=1;h=5&&((e||!n&&5===r)&&(h.push(r,0,e,s),r=6),n&&(h.push(r,n,0,s),r=6)),e="";},a=0;a"===t?(r=1,e=""):e=t+e[0]:u?t===u?u="":e+=t:'"'===t||"'"===t?u=t:">"===t?(p(),r=1):r&&("="===t?(r=5,s=e,e=""):"/"===t&&(r<5||">"===n[a][l+1])?(p(),3===r&&(h=h[0]),r=h,(h=h[0]).push(2,0,r),r=0):" "===t||"\t"===t||"\n"===t||"\r"===t?(p(),r=2):e+=t),3===r&&"!--"===e&&(r=4,h=h[0]);}return p(),h}(s)),r),arguments,[])).length>1?r:r[0]} +function serializeEvent(event) { + const data = {}; + + if (event.type in eventTransforms) { + Object.assign(data, eventTransforms[event.type](event)); + } + + const target = event.target; + if (target.tagName in targetTransforms) { + targetTransforms[target.tagName].forEach((trans) => + Object.assign(data, trans(target)) + ); + } + + return data; +} + +const targetTransformCategories = { + hasValue: (target) => ({ + value: target.value, + }), + hasCurrentTime: (target) => ({ + currentTime: target.currentTime, + }), + hasFiles: (target) => { + if (target?.type == "file") { + return { + files: Array.from(target.files).map((file) => ({ + lastModified: file.lastModified, + name: file.name, + size: file.size, + type: file.type, + })), + }; + } else { + return {}; + } + }, +}; + +const targetTagCategories = { + hasValue: ["BUTTON", "INPUT", "OPTION", "LI", "METER", "PROGRESS", "PARAM"], + hasCurrentTime: ["AUDIO", "VIDEO"], + hasFiles: ["INPUT"], +}; + +const targetTransforms = {}; + +Object.keys(targetTagCategories).forEach((category) => { + targetTagCategories[category].forEach((type) => { + const transforms = targetTransforms[type] || (targetTransforms[type] = []); + transforms.push(targetTransformCategories[category]); + }); +}); + +const eventTransformCategories = { + clipboard: (event) => ({ + clipboardData: event.clipboardData, + }), + composition: (event) => ({ + data: event.data, + }), + keyboard: (event) => ({ + altKey: event.altKey, + charCode: event.charCode, + ctrlKey: event.ctrlKey, + key: event.key, + keyCode: event.keyCode, + locale: event.locale, + location: event.location, + metaKey: event.metaKey, + repeat: event.repeat, + shiftKey: event.shiftKey, + which: event.which, + }), + mouse: (event) => ({ + altKey: event.altKey, + button: event.button, + buttons: event.buttons, + clientX: event.clientX, + clientY: event.clientY, + ctrlKey: event.ctrlKey, + metaKey: event.metaKey, + pageX: event.pageX, + pageY: event.pageY, + screenX: event.screenX, + screenY: event.screenY, + shiftKey: event.shiftKey, + }), + pointer: (event) => ({ + pointerId: event.pointerId, + width: event.width, + height: event.height, + pressure: event.pressure, + tiltX: event.tiltX, + tiltY: event.tiltY, + pointerType: event.pointerType, + isPrimary: event.isPrimary, + }), + selection: () => { + return { selectedText: window.getSelection().toString() }; + }, + touch: (event) => ({ + altKey: event.altKey, + ctrlKey: event.ctrlKey, + metaKey: event.metaKey, + shiftKey: event.shiftKey, + }), + ui: (event) => ({ + detail: event.detail, + }), + wheel: (event) => ({ + deltaMode: event.deltaMode, + deltaX: event.deltaX, + deltaY: event.deltaY, + deltaZ: event.deltaZ, + }), + animation: (event) => ({ + animationName: event.animationName, + pseudoElement: event.pseudoElement, + elapsedTime: event.elapsedTime, + }), + transition: (event) => ({ + propertyName: event.propertyName, + pseudoElement: event.pseudoElement, + elapsedTime: event.elapsedTime, + }), +}; + +const eventTypeCategories = { + clipboard: ["copy", "cut", "paste"], + composition: ["compositionend", "compositionstart", "compositionupdate"], + keyboard: ["keydown", "keypress", "keyup"], + mouse: [ + "click", + "contextmenu", + "doubleclick", + "drag", + "dragend", + "dragenter", + "dragexit", + "dragleave", + "dragover", + "dragstart", + "drop", + "mousedown", + "mouseenter", + "mouseleave", + "mousemove", + "mouseout", + "mouseover", + "mouseup", + ], + pointer: [ + "pointerdown", + "pointermove", + "pointerup", + "pointercancel", + "gotpointercapture", + "lostpointercapture", + "pointerenter", + "pointerleave", + "pointerover", + "pointerout", + ], + selection: ["select"], + touch: ["touchcancel", "touchend", "touchmove", "touchstart"], + ui: ["scroll"], + wheel: ["wheel"], + animation: ["animationstart", "animationend", "animationiteration"], + transition: ["transitionend"], +}; + +const eventTransforms = {}; + +Object.keys(eventTypeCategories).forEach((category) => { + eventTypeCategories[category].forEach((type) => { + eventTransforms[type] = eventTransformCategories[category]; + }); +}); + /*! * https://github.com/Starcounter-Jack/JSON-Patch * (c) 2017 Joachim Wester @@ -1388,186 +1569,38 @@ var jsonpatch = Object.assign({}, core, duplex, { unescapePathComponent }); -function serializeEvent(event) { - const data = {}; - - if (event.type in eventTransforms) { - Object.assign(data, eventTransforms[event.type](event)); - } +function useJsonPatchCallback(initial) { + const model = react.useRef(initial); + const forceUpdate = useForceUpdate(); - const target = event.target; - if (target.tagName in targetTransforms) { - targetTransforms[target.tagName].forEach((trans) => - Object.assign(data, trans(target)) - ); - } + const applyPatch = react.useCallback( + (pathPrefix, patch) => { + if (pathPrefix) { + patch = patch.map((op) => + Object.assign({}, op, { path: pathPrefix + op.path }) + ); + } + // Always return a newDocument because React checks some attributes of the model + // (e.g. model.attributes.style is checked for identity) + model.current = jsonpatch.applyPatch( + model.current, + patch, + false, + false, + true + ).newDocument; + forceUpdate(); + }, + [model] + ); - return data; + return [model.current, applyPatch]; } -const targetTransformCategories = { - hasValue: (target) => ({ - value: target.value, - }), - hasCurrentTime: (target) => ({ - currentTime: target.currentTime, - }), - hasFiles: (target) => { - if (target?.type == "file") { - return { - files: Array.from(target.files).map((file) => ({ - lastModified: file.lastModified, - name: file.name, - size: file.size, - type: file.type, - })), - }; - } else { - return {}; - } - }, -}; - -const targetTagCategories = { - hasValue: ["BUTTON", "INPUT", "OPTION", "LI", "METER", "PROGRESS", "PARAM"], - hasCurrentTime: ["AUDIO", "VIDEO"], - hasFiles: ["INPUT"], -}; - -const targetTransforms = {}; - -Object.keys(targetTagCategories).forEach((category) => { - targetTagCategories[category].forEach((type) => { - const transforms = targetTransforms[type] || (targetTransforms[type] = []); - transforms.push(targetTransformCategories[category]); - }); -}); - -const eventTransformCategories = { - clipboard: (event) => ({ - clipboardData: event.clipboardData, - }), - composition: (event) => ({ - data: event.data, - }), - keyboard: (event) => ({ - altKey: event.altKey, - charCode: event.charCode, - ctrlKey: event.ctrlKey, - key: event.key, - keyCode: event.keyCode, - locale: event.locale, - location: event.location, - metaKey: event.metaKey, - repeat: event.repeat, - shiftKey: event.shiftKey, - which: event.which, - }), - mouse: (event) => ({ - altKey: event.altKey, - button: event.button, - buttons: event.buttons, - clientX: event.clientX, - clientY: event.clientY, - ctrlKey: event.ctrlKey, - metaKey: event.metaKey, - pageX: event.pageX, - pageY: event.pageY, - screenX: event.screenX, - screenY: event.screenY, - shiftKey: event.shiftKey, - }), - pointer: (event) => ({ - pointerId: event.pointerId, - width: event.width, - height: event.height, - pressure: event.pressure, - tiltX: event.tiltX, - tiltY: event.tiltY, - pointerType: event.pointerType, - isPrimary: event.isPrimary, - }), - selection: () => { - return { selectedText: window.getSelection().toString() }; - }, - touch: (event) => ({ - altKey: event.altKey, - ctrlKey: event.ctrlKey, - metaKey: event.metaKey, - shiftKey: event.shiftKey, - }), - ui: (event) => ({ - detail: event.detail, - }), - wheel: (event) => ({ - deltaMode: event.deltaMode, - deltaX: event.deltaX, - deltaY: event.deltaY, - deltaZ: event.deltaZ, - }), - animation: (event) => ({ - animationName: event.animationName, - pseudoElement: event.pseudoElement, - elapsedTime: event.elapsedTime, - }), - transition: (event) => ({ - propertyName: event.propertyName, - pseudoElement: event.pseudoElement, - elapsedTime: event.elapsedTime, - }), -}; - -const eventTypeCategories = { - clipboard: ["copy", "cut", "paste"], - composition: ["compositionend", "compositionstart", "compositionupdate"], - keyboard: ["keydown", "keypress", "keyup"], - mouse: [ - "click", - "contextmenu", - "doubleclick", - "drag", - "dragend", - "dragenter", - "dragexit", - "dragleave", - "dragover", - "dragstart", - "drop", - "mousedown", - "mouseenter", - "mouseleave", - "mousemove", - "mouseout", - "mouseover", - "mouseup", - ], - pointer: [ - "pointerdown", - "pointermove", - "pointerup", - "pointercancel", - "gotpointercapture", - "lostpointercapture", - "pointerenter", - "pointerleave", - "pointerover", - "pointerout", - ], - selection: ["select"], - touch: ["touchcancel", "touchend", "touchmove", "touchstart"], - ui: ["scroll"], - wheel: ["wheel"], - animation: ["animationstart", "animationend", "animationiteration"], - transition: ["transitionend"], -}; - -const eventTransforms = {}; - -Object.keys(eventTypeCategories).forEach((category) => { - eventTypeCategories[category].forEach((type) => { - eventTransforms[type] = eventTransformCategories[category]; - }); -}); +function useForceUpdate() { + const [, updateState] = react.useState(); + return react.useCallback(() => updateState({}), []); +} const html = htm.bind(react.createElement); const LayoutConfigContext = react.createContext({ @@ -1745,39 +1778,6 @@ function loadImportSource$1(config, importSource) { }); } -function useJsonPatchCallback(initial) { - const model = react.useRef(initial); - const forceUpdate = useForceUpdate(); - - const applyPatch = react.useCallback( - (pathPrefix, patch) => { - if (pathPrefix) { - patch = patch.map((op) => - Object.assign({}, op, { path: pathPrefix + op.path }) - ); - } - // Always return a newDocument because React checks some attributes of the model - // (e.g. model.attributes.style is checked for identity) - model.current = jsonpatch.applyPatch( - model.current, - patch, - false, - false, - true - ).newDocument; - forceUpdate(); - }, - [model] - ); - - return [model.current, applyPatch]; -} - -function useForceUpdate() { - const [, updateState] = react.useState(); - return react.useCallback(() => updateState({}), []); -} - function mountLayout(mountElement, layoutProps) { reactDom.render(react.createElement(Layout, layoutProps), mountElement); } diff --git a/docs/source/examples/snake_game.py b/docs/source/examples/snake_game.py index 1f42879fb..313ff4a83 100644 --- a/docs/source/examples/snake_game.py +++ b/docs/source/examples/snake_game.py @@ -51,9 +51,7 @@ def GameLoop(grid_size, block_scale, set_game_state): food, set_food = use_snake_food(grid_size, snake) grid = create_grid(grid_size, block_scale) - grid_events = grid["eventHandlers"] = idom.Events() - @grid_events.on("KeyDown", prevent_default=True) def on_direction_change(event): if hasattr(Direction, event["key"]): maybe_new_direction = Direction[event["key"]].value @@ -63,6 +61,8 @@ def on_direction_change(event): if direction_vector_sum != (0, 0): direction.current = maybe_new_direction + grid_wrapper = idom.html.div({"onKeyDown": on_direction_change}, grid) + assign_grid_block_color(grid, food, "blue") for location in snake: @@ -101,7 +101,7 @@ async def animate(): set_snake(new_snake) - return grid + return grid_wrapper def use_snake_food(grid_size, current_snake):