Monkey Function v2 #174

balazsbajorics · 2020-06-29T14:57:37Z

Fixes #149

Problem:
The antd Date Picker, once inserted, is not selectable on the Canvas.

Fix:
The problem was that the date picker didn't get the proper data-uid so we couldn't find it in the DOM. The reason why it didn't get that uid was that the existing monkey funciton implementation didn't cover the following case:

const ComponentA = (props) => <ComponentB>Hello</ComponentB>
const ComponentB = (props) => <div>{props.children}</div>

const App = (props) => <div><ComponentA data-uid="cica" /></div>

The above code would turn into the following DOM:

<div><div>Hello</div></div>

But what we want is this DOM:

<div><div data-uid="cica">Hello</div></div>

After poking at the monkey system I realized that this case was not supported at a conceptual level: the mokney would only run if there was already a data-uid to begin with, AND it would use the data-uid found at the time React.createElement is called. The latter is important and wrong because the entire monkey is built around the idea that we switch out the props of root elements before we give them to React. If we bind an old value of data-uid to an element, then later when we try to switch it, this switching will have no effect if we are not reading the latest of props['data-uid'].

Commit Details:

Rewrote the monkey patcher
Added a suite of unit tests specifically testing various monkey edge cases. Please let me know if I missed something here!
Used remaining goat blood to paint the shrine

Important Note
Unfortunately this PR increases the render counts :(
I believe the reason is that I am wrapping exotic components into a wrapper component, adds one more render count to each exotic component (fragments, memos, context, etc).
The bigger picture problem is that the monkey is (and always was) also monkeying our own UI code, but the previous version of the monkey was written in a way that it would instantly bail out if it did not find a data-uid whereas the new monkey aggressively wraps things because it anticipates a data-uid that only arrives halfway through the reconciliation (this is the fix for the original issue).

I believe the solution for the performance going forward is to figure out a way to disable the monkey for our own UI components, but I did not want to further bloat this Pr by the solution to that. I think technically the easiest solution (which would require a lot of labor unfortunately) would be to create a new pragma function that disables the monkey. We would need to add this pragma to every file that contains editor UI (all the inspector, navigator, filebrowser etc source files)

editor/package.json

balazsbajorics · 2020-06-29T14:59:01Z

editor/src/components/canvas/ui-jsx-canvas.spec.tsx

-  expect(errorsReportedSpyEnabled.length).toBe(0)
-  expect(errorsReportedSpyDisabled.length).toBe(0)
+  if (errorsReportedSpyEnabled.length > 0) {
+    throw new Error(`Canvas Tests, Spy Enabled: Errors reported: ${errorsReportedSpyEnabled}`)


this will still fail the test if there are errors, but it will also helpfully print those errors so it is easier to debug a test

editor/src/components/canvas/ui-jsx-canvas.tsx

Rheeseyb · 2020-06-29T16:52:03Z

editor/src/core/performance/performance-regression-tests.spec.tsx

@@ -59,8 +59,8 @@ describe('React Render Count Tests - ', () => {
    )

    const renderCountAfter = renderResult.getNumberOfRenders()
-    expect(renderCountAfter - renderCountBefore).toBeGreaterThan(720) // if this breaks, GREAT NEWS but update the test please :)
-    expect(renderCountAfter - renderCountBefore).toBeLessThan(740)
+    expect(renderCountAfter - renderCountBefore).toBeGreaterThan(855) // if this breaks, GREAT NEWS but update the test please :)


Does that tie to a similar change in performance?

I see a ~10 percent decrease in dragging FPS, but it is the local not-quite-production environment, so maybe the real difference is smaller.
in any case, we definitely should follow this up by disabling the monkey on the editor UI

I've created #181 and moved it into todo.

seanparsons

Nice enumeration of the test cases.

editor/src/utils/canvas-react-utils.ts

Co-authored-by: RheeseyB <1044774+Rheeseyb@users.noreply.github.com>

editor/src/utils/canvas-react-utils.ts

Rheeseyb · 2020-06-30T10:02:36Z

editor/src/utils/canvas-react-utils.ts

+
+        return realCreateElement(type, newProps)
+      } else {
+        const mangledChildren = React.Children.map(p.children, (child) => {


We need to first check if the "children" is a single element or an array, otherwise we can break the element

I believe react.children.map takes care of that

Nah unfortunately it doesn't. This was something that caused us a lot of pain with antd when we discovered this issue facebook/react#13355

ughhh. fixed!

Rheeseyb · 2020-06-30T10:04:41Z

editor/src/utils/canvas-react-utils.spec.tsx

+    `)
+  })
+
+  it('works with a silly render prop', () => {


So so horrible! Great job catching and covering this one

* store the entire module object as the codeResultCache * new transformCssNodeModule that runs the css append as a side effect of require * evalresult contains a module instead of it being a module

* fix color picker's state * Update color-picker.tsx * fix dirty state name and type

balazsbajorics · 2020-06-30T11:01:43Z

@Rheeseyb I updated the PR, could you take another look?

Rheeseyb · 2020-06-30T11:10:52Z

editor/package.json

@@ -155,7 +155,7 @@
    "@testing-library/react": "9.1.4",
    "@testing-library/react-hooks": "2.0.1",
    "@types/chai": "3.5.1",
-    "@types/chroma-js": "1.4.3",
+    "@types/chroma-js": "2.0.0",


I think github is having a glitch. this is on master

Looks like merging master fixed.

balazsbajorics added 23 commits June 24, 2020 21:05

adding test for silly render props

e49aa4b

adding specific test for the react monkey

2632d9e

Update canvas-react-utils.spec.tsx

cfd1ed8

Update canvas-react-utils.spec.tsx

f29c992

Update canvas-react-utils.spec.tsx

0c21ebe

Merge branch 'master' into fix/monkey-oh

5aacde0

Update canvas-react-utils.spec.tsx

28152ce

Update canvas-react-utils.spec.tsx

e5e2f4a

almost working new monkey function

44c7c0d

Update canvas-react-utils.spec.tsx

4777992

Update canvas-react-utils.spec.tsx

7e68f95

adding missing antd dependency

22fb809

checkpoint

8aefa35

fixing canvas tests, breaking antd test :(

ec40c79

Update canvas-react-utils.ts

b080200

adding a React.isValidElement for good measure

b446737

using cloneElement in one more place

35b78df

Merge branch 'master' into fix/monkey-oh

f670f43

only mangle Fragments, providers and context

ba85791

Update performance-regression-tests.spec.tsx

9f68f79

Update canvas-react-utils.ts

abe3ac7

removing old code

8ef5ecc

Update canvas-react-utils.ts

134c4c2

balazsbajorics requested review from Rheeseyb, alecmolloy, enidemi and seanparsons June 29, 2020 14:57

balazsbajorics commented Jun 29, 2020

View reviewed changes

editor/package.json Outdated Show resolved Hide resolved

balazsbajorics commented Jun 29, 2020

View reviewed changes

editor/src/components/canvas/ui-jsx-canvas.tsx Outdated Show resolved Hide resolved

enidemi approved these changes Jun 29, 2020

View reviewed changes

Rheeseyb reviewed Jun 29, 2020

View reviewed changes

seanparsons approved these changes Jun 30, 2020

View reviewed changes

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Show resolved Hide resolved

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Outdated Show resolved Hide resolved

Update editor/package.json

71a4c08

Co-authored-by: RheeseyB <1044774+Rheeseyb@users.noreply.github.com>

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Outdated Show resolved Hide resolved

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Outdated Show resolved Hide resolved

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Outdated Show resolved Hide resolved

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Outdated Show resolved Hide resolved

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

editor/src/utils/canvas-react-utils.ts Show resolved Hide resolved

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

balazsbajorics and others added 9 commits June 30, 2020 12:13

Fix/allow side effects at require time (#165)

50c8f80

* store the entire module object as the codeResultCache * new transformCssNodeModule that runs the css append as a side effect of require * evalresult contains a module instead of it being a module

fix color picker's state (#169)

e64314b

* fix color picker's state * Update color-picker.tsx * fix dirty state name and type

removing unused monkeyEscapeHatch

15ce320

removing the monkey-oh-monkey crap

4340476

Update canvas-react-utils.ts

7fd9d58

making the code read a bit more clearly

2fd3760

Update canvas-react-utils.ts

0da1537

more honest cheating with the type casting here

206546c

adding all of the question marks

8a4d65d

Rheeseyb reviewed Jun 30, 2020

View reviewed changes

maltenuhn mentioned this pull request Jun 30, 2020

Prevent monkey descent into the editor UI #181

Open

balazsbajorics added 2 commits June 30, 2020 14:58

Merge branch 'master' into fix/monkey-oh

205772b

differentiate between single and array children for exotic elements

a1b8f5f

Rheeseyb approved these changes Jun 30, 2020

View reviewed changes

balazsbajorics merged commit b66f74d into master Jun 30, 2020

balazsbajorics deleted the fix/monkey-oh branch June 30, 2020 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monkey Function v2 #174

Monkey Function v2 #174

balazsbajorics commented Jun 29, 2020

balazsbajorics Jun 29, 2020

Rheeseyb Jun 29, 2020

seanparsons Jun 30, 2020

balazsbajorics Jun 30, 2020

maltenuhn Jun 30, 2020

seanparsons left a comment

Rheeseyb Jun 30, 2020

balazsbajorics Jun 30, 2020

Rheeseyb Jun 30, 2020

balazsbajorics Jun 30, 2020

Rheeseyb Jun 30, 2020

balazsbajorics commented Jun 30, 2020

Rheeseyb Jun 30, 2020

balazsbajorics Jun 30, 2020

Rheeseyb Jun 30, 2020

Monkey Function v2 #174

Monkey Function v2 #174

Conversation

balazsbajorics commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanparsons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balazsbajorics commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment