Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to react v18, mobx-react v9 #3502

Merged
merged 7 commits into from
Sep 8, 2023
Merged

Update to react v18, mobx-react v9 #3502

merged 7 commits into from
Sep 8, 2023

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Feb 5, 2023

This could be a major version bump to the jbrowse codebase, similar to how the update to jbrowse 2 was based on dependencies.

A major reason for a major bump due to this would be using hydrateRoot to hydrate the RPC rendered stuff, this would fail if a user had less than react 18 installed

We could just use react-dom "hydrate" and "render" in react 18 instead of hydrateRoot/createRoot but this spams the console with lots of warnings

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 5, 2023
@cmdcolin cmdcolin force-pushed the react_18 branch 2 times, most recently from 326627e to 15719ce Compare February 5, 2023 20:31
@cmdcolin cmdcolin marked this pull request as draft February 5, 2023 20:37
@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #3502 (2cc368e) into main (9374e84) will decrease coverage by 0.15%.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##             main    #3502      +/-   ##
==========================================
- Coverage   64.50%   64.36%   -0.15%     
==========================================
  Files        1004     1004              
  Lines       29811    29868      +57     
  Branches     7147     7158      +11     
==========================================
- Hits        19231    19225       -6     
- Misses      10415    10478      +63     
  Partials      165      165              
Files Changed Coverage Δ
...ementTypes/renderers/CircularChordRendererType.tsx 10.00% <ø> (ø)
...TrackSelectorWidget/components/tree/TrackLabel.tsx 80.00% <ø> (ø)
products/jbrowse-desktop/src/index.tsx 0.00% <0.00%> (ø)
products/jbrowse-desktop/src/rootModel/index.ts 47.05% <ø> (ø)
products/jbrowse-react-app/src/createModel.ts 0.00% <ø> (ø)
products/jbrowse-react-app/src/createViewState.ts 0.00% <0.00%> (ø)
products/jbrowse-react-app/src/rootModel/index.ts 32.83% <ø> (ø)
...-linear-genome-view/src/createModel/createModel.ts 38.46% <ø> (ø)
...se-react-linear-genome-view/src/createViewState.ts 57.69% <ø> (ø)
products/jbrowse-web/src/index.tsx 0.00% <0.00%> (ø)
... and 9 more

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin added dependencies Pull requests that update a dependency file and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Feb 6, 2023
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 7, 2023
@cmdcolin cmdcolin force-pushed the react_18 branch 4 times, most recently from 7f48aab to 2e3593c Compare February 17, 2023 09:38
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 17, 2023
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 20, 2023
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 20, 2023
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 21, 2023
@cmdcolin cmdcolin force-pushed the react_18 branch 4 times, most recently from 40bc531 to 0cdf343 Compare February 24, 2023 23:48
@cmdcolin cmdcolin force-pushed the react_18 branch 2 times, most recently from 73f94c1 to a59b7fb Compare March 3, 2023 15:28
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Mar 3, 2023

this branch updates to @mui/x-data-grid v6 now also. it has some props renamed, but is otherwise pretty similar in behavior

@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 3, 2023
@cmdcolin cmdcolin changed the title Update to react 18 Update to react v18 and @mui/x-data-grid v6 Mar 3, 2023
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Sep 8, 2023

I updated this this PR again, motivated by increasing amounts of flaky test failures which are mentioned in #3624

My understanding is that react 18/updated testing-library can help with this.

I additionally added a method that can be used for conditionally rendering with react 18 react-dom/client::hydrateRoot, or fallback to the old way, by checking for a property called hydrateFn on the root model.

this allows consumers of embedded components e.g. @jbrowse/react-linear-genome-view to add a "hydrateFn" paramter to supply the hydrateRoot function to createModel, and upgrade themselves to react 18. If this hydrateFn function exists on the root model, then it is used by the rpc functions. This way, @jbrowse/core can stay agnostic to react version, and doesn't require a major version bump/require every client consumer of the library to update to react 18

@cmdcolin cmdcolin marked this pull request as ready for review September 8, 2023 16:12
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 8, 2023
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Sep 8, 2023

note that for a long time we have probably ignored a console.warn (in many current existing versions) in the LGV something to the effect of

"Warning: Expected server HTML to contain a matching <rect> in <svg>."

I think this is related to Overlay rect of the selected feature. In this branch though, instead of a small console.warn, it is an error overlay.

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Sep 8, 2023

that error overlay would be dev mode only...and could be fixed separately. otherwise I think this PR is probably ready

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 8, 2023
Fixup testid and add more timeout

update react 18 server side rendering attempt based on https://stackoverflow.com/questions/73459382/react-18-async-way-to-unmount-root

Bump lock

Misc

Misc

Add a useMemo

Updates

Update snaps

Misc test fixes

Bump mobx-react peer dep

Update snaps

Misc

Update snaps
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 8, 2023
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 8, 2023
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Sep 8, 2023

might give this a merge. if it is problematic it can be partially or fully reverted, but i think it will hopefully help with intermittent test fails, and keeps us up to date with deps

@cmdcolin cmdcolin added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Sep 8, 2023
@cmdcolin cmdcolin merged commit 7df90d0 into main Sep 8, 2023
@cmdcolin cmdcolin changed the title Update to react v18 Update to react v18, mobx-react v9 Sep 8, 2023
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Sep 9, 2023

note that this also enabled <React.StrictMode>. if there are any issues in apollo or other plugins let me know, could turn strict mode off or fully revert the 18 upgrade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant