CE-179 Updating to React 17

[CedricVanhaverbeke]

I'd like to update the scratch GUI to React 17 to make use of new features. I'm planning to do this and keep the tests (both unit and integration) working.

Is this something that is desired? Or should I move away from this idea?

[apple502j]

See also: LLK/eslint-config-scratch#76

[paulkaplan]

@CedricVanhaverbeke yes that is something I was just starting to work on!

The blocking issue is that scratch-www has to be updated to use react 17 first, since it provides the peer dependency of react to scratch-gui. But that can't happen until we make a few changes in GUI first. My plan was roughly:

• Change deprecated lifecycle to their UNSAFE_ prefixed version
• Locally test the GUI using react 17, fix any bugs that come up (I noticed one issue with the dropdown menus not opening, I think it is an event sequencing thing, not sure if 17 specific or one of the 16.X updates)
• Update GUI to allow a peer dependency of 17, but do not use any new lifecycle methods or features yet.
• Update WWW to use react 17
• Come back and update the UNSAFE lifecycle methods, etc.

The same process will be going on with scratch-paint, since it also relies on the same peer dependency of react.

I'm currently in the process of updating our eslint config rules to allow the unsafe lifecycle methods (what @apple502j linked). The most useful thing you could contribute right now would be to try updating to react 17 locally and testing out the GUI to see if there are any bugs. A PR fixing the dropdowns not opening would be appreciated, and filing or fixing any other issues you find would be good too.

[paulkaplan]

One other thing @CedricVanhaverbeke or @apple502j, the team is trying to start getting up to speed with react hooks and other newer react features. One thing that would also be helpful is if you could post an example of a part of our code that might be made better if we were to use hooks. Just a snippet here in this issue would be great to share with the team.

[CedricVanhaverbeke]

To be fair, We now have a working version of the Scratch GUI running with React 17, with all tests succeeding. So maybe you could focus on updating scratch-www ? Can I create a PR with our modifications in the meantime?

Maybe @DemianD might provide more insight to this. We don't have to switch everything to hooks. The React team itself suggests to leave exisitng features as is, and when adding new features start using hooks.

It's quite hard for me to just create a snippet with changed code. I suggest that you could start small with getting to know the different hooks: (https://reactjs.org/docs/hooks-intro.html). The list is surprisingly small.

An example of the impact of hooks: It is not needed to pass everything you need from the redux store in a mapStateToProps function anymore. You could just write something like this in a component:

import { useSelector } from 'react-redux'

const Component = () => {
   const somethingFromStore = useSelector((store) => store.something);
}

I woud not advise to change everything to React hooks, you can upgrade along the way if you desire.

[paulkaplan]

I'm surprised, I tried upgrading to react 17 and couldn't open the file/edit menus. Have any suggestions?

[apple502j]

It could be dependency problems, let's see

[DemianD]

Hello

I've upgraded our internal scratch-gui to React 17 (I work for the same company as @CedricVanhaverbeke) The file/edit menus are indeed a problem.

When you click on a file button in the navigation, a "close" event is directly sent. I've changed the addListeners from this:

https://github.com/LLK/scratch-gui/blob/a14f17ba744471a8568bb2a34b08c7c71c106278/src/containers/menu.jsx#L27-L29

to this:

addListeners () {
+        setTimeout(() => {
            document.addEventListener('mouseup', this.handleClick); 
+        }, 1);
} 

This is just a quick fix, and is of course not something I'd encourage. Probably related to this change: https://reactjs.org/blog/2020/10/20/react-v17.html#changes-to-event-delegation

Some more information:

Change deprecated lifecycle to their UNSAFE_ prefixed version

This is something that we did, but it's still not required until React v18. The following script will update all of them:

npx react-codemod rename-unsafe-lifecycles <path>

To make sure ESLint doesn't complain, you should add this to the rules section

    rules: {
        ...
+        'camelcase': [2, {
+            properties: 'never',
+            allow: ["^UNSAFE_"]
+        }],

Enzyme

Enzyme doesn't support React 17 at this moment. We're making use of @wojtekmaj/enzyme-adapter-react-17 until Enzyme supports React 17.

Updated dependencies:

• react
• react-dom
• react-contextmenu
• Everything from @babel and babel
• jest
• jest-junit
• react-test-renderer
• enzyme

Waiting for tests to complete:

Most tests are failing because the test runner doesn't know when the test has completed. This is probably because we've upgraded to a new jest version. Our solution is to make use of the done-callback.

+ test('if the fontsLoaded prop is false, project data is never loaded', done => {
        ...
        process.nextTick(() => {
            expect(mockedOnLoadedProject).toHaveBeenCalledTimes(0);
+            done();
        });
});

Snapshots:

Snapshots will now have MockFunction instead of undefined when using a spy function.

     <div
        className=""
-       onClick={undefined}
+       onClick={[MockFunction]}
       role="button"
    >

Undefined props are now removed from the snapshots.

      <img
-       className={undefined}
        draggable={false}
        src="test-file-stub"
      />

I hope this helps.