h5web
h5web copied to clipboard
Upgrade React to v18 and R3F to v8
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 useSuspense
(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'sCanvas
. 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 forSuspense
thanreact-suspense-fetch
; it's developed by the R3F team and used internally in R3F itself
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?
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 😢
@bmaranville I'm getting some warnings in the H5Wasm demo when I select a simple HDF5 file.
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'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.
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.
@bmaranville I'm getting some warnings in the H5Wasm demo when I select a simple HDF5 file.
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.ccNot 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.
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.
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.
Oh it must be <StrictMode>
!
- 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."
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.
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!
/approve
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.
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.
/approve