h5web icon indicating copy to clipboard operation
h5web copied to clipboard

Upgrade React to v18 and R3F to v8

Open axelboc opened this issue 2 years ago • 13 comments

The big change with React 18, obviously, is the createRoot API that replaces ReactDOM.render. In terms of code, this affects the demo but most importantly the Html component (I think the new API makes the code clearer, actually). In terms of performance, this means we can now opt-in to concurrent rendering features (see below). That being said, there should already be some performance improvements under the hood, especially since R3F already makes use of such features internally.

There's a bunch of peer dependency warnings that should go away little by little, as usual, notably with Storybook (there's a release in the works), ~react-slider, react-reflex~, etc. Talking about react-reflex, this library is affected by the breaking change in @types/react that consists in the removal of the implicit children prop from component types, like Component. So for instance, const ReflexContainer: React.Component<Props, any> is supposed to now be written as const ReflexContainer: React.Component<PropsWithChildren<Props>, any>. ~I've added @ts-expect-error comments for now.~ This is now fixed.

In R3F v8, the breaking change that affects our codebase is the sourceEvent key in event objects being renamed to nativeEvent.

Obviously there's a lot more to these major releases that we can benefit from or should watch out for (I will put this list in a discussion thread once merged):

  • useTransition to improve the way we use Suspense (e.g. by not hiding the current slice while the next one is loading)
  • useDeferredValue to improve performance on a case-by-case basis by deferring heavy, low-priority state updates
  • [x] changes to colour management in Three v0.139 (~we're currently blocked at v0.135 due to a breaking change~ #1170), thanks to a change in Three 0,139, we no longer have to set the linear prop on R3F's Canvas. I think we end up with fewer conversions of colours under the hood.
  • updating the R3F camera manually on resize to (potentially) fix the pixel resizing issue 🎉
  • using the attach API for declaring buffer attributes
  • testing R3F components (the R3F test renderer has been available for a while, but it's worth keeping in mind)
  • suspend-react, a lower-level library for Suspense than react-suspense-fetch; it's developed by the R3F team and used internally in R3F itself

axelboc avatar May 13 '22 13:05 axelboc

I've rebased and updated a few packages. react-reflex, react-use and react-slider now allow React 18 as peer dep.

I've noticed that the H5Wasm provider no longer works. I get warnings and errors when I try to open a file. I tried to debug the issue but to no avail. Perhaps it comes from the internals of H5Wasm?

image

axelboc avatar May 25 '22 13:05 axelboc

TODO

  • [x] Fix feature tests (lots of act warnings)
  • [x] Fix H5Wasm demo
  • [x] ~Backwards compatibility with React 17: Html element should fallback to use legacy API~ R3F is not backwards compatible 😢

axelboc avatar Jun 08 '22 08:06 axelboc

@bmaranville I'm getting some warnings in the H5Wasm demo when I select a simple HDF5 file.

image

After the warnings, I get a name not defined error followed by a white screen. The error seems to be coming from https://github.com/usnistgov/h5wasm/blob/main/src/hdf5_util.cc

Not sure how this is linked to upgrading to React 18... Any clue?

axelboc avatar Jun 09 '22 13:06 axelboc

I've reverted the switch to createRoot in Html so we can remain backward compatible with React 17. It seems to work, but the downside is that we'll get lots of warnings in the console on apps running React 18. I guess we can live with it until we've migrated everything, and then we'll drop support for React 17 in a new major version.

axelboc avatar Jun 09 '22 13:06 axelboc

Change of plan: as discussed, it might be best to consider maintaining two release branches in case some apps struggle to update to React 18. While we decide and set this up, I'll keep this PR open as draft. I'll, again, remove support for React 17 in this branch and backport all the improvements to the feature tests into the main branch.


UPDATE: I was unable to backport the changes to the tests :disappointed: -- too many failures and I don't want to spend more time on this trying to come up with a middle-ground solution.

axelboc avatar Jun 10 '22 07:06 axelboc

@bmaranville I'm getting some warnings in the H5Wasm demo when I select a simple HDF5 file.

image

After the warnings, I get a name not defined error followed by a white screen. The error seems to be coming from https://github.com/usnistgov/h5wasm/blob/main/src/hdf5_util.cc

Not sure how this is linked to upgrading to React 18... Any clue?

I will try to build the react 18 branch and see what is causing the error.

bmaranville avatar Jun 10 '22 10:06 bmaranville

It seems to be calling file.close in the api.cleanUp method when the file is already closed... I suspect it is because of a change in how useEffect is handled in react 18 (it is apparently called twice sometimes) - see https://www.techiediaries.com/react-18-useeffect/

EDIT: Nope - I was mistaken. It's not being called twice, and it's failing on the first call.

What is odd is that it is calling the api.cleanUp method immediately when the file is opened - that shouldn't be happening. It doesn't do that in react 17. If it tries to close the file while it is actively being read (when there are active handles to objects in the file) it will fail with an error like the one you showed.

bmaranville avatar Jun 10 '22 10:06 bmaranville

I can verify that the h5wasm loader works for a pre-built version of the demo, with react 18. So it does seem to be related to the change in useeffect in react 18, when running in development mode.

bmaranville avatar Jun 10 '22 10:06 bmaranville

Oh it must be <StrictMode>!

axelboc avatar Jun 10 '22 12:06 axelboc

  • https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#updates-to-strict-mode
  • https://github.com/reactwg/react-18/discussions/18

Not quite sure how to work around it to still get a proper clean up. I'll investigate. Thanks!

EDIT Actually, it's exactly the example they provide. I'll try to re-initialise the API inside the effect to see what happens. But maybe I'll run into the issue you mention anyway: "If it tries to close the file while it is actively being read (when there are active handles to objects in the file) it will fail with an error like the one you showed."

axelboc avatar Jun 10 '22 12:06 axelboc

Also you shouldn't close the file until you are done working with it. You would have to open and get a new file handle to e.g. navigate or read any datasets if you close it. close should only be called during cleanup when e.g. you want to open a new, different file.

bmaranville avatar Jun 10 '22 12:06 bmaranville

All good, by putting the api instance in a state, strict mode doesn't call the effect twice, so doesn't call the clean-up. I've also confirmed that changing the props of H5WasmProvider after a timeout works. Thanks for your help!

axelboc avatar Jun 10 '22 12:06 axelboc

/approve

axelboc avatar Aug 10 '22 12:08 axelboc

Attn @PeterC-DLS: I've just rebased this branch on v7. From now on, I'll try to rebase it only after stable releases.

I'm just doing a basic sanity check locally with pnpm start before force-pushing, and then the CI runs as usual—I'm not testing this branch in production projects yet—so do let me know if you notice issues.

axelboc avatar Feb 13 '23 08:02 axelboc

I've rebased and fixed a few things, but I had to skip a couple of tests for the CI to pass. I'll see if upgrading Testing Library helps and investigate a bit more.

The plan is to merge this and release a new major asap.

axelboc avatar Sep 07 '23 14:09 axelboc

/approve

axelboc avatar Sep 07 '23 14:09 axelboc