fix: revert breaking React 18 changes #913
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sehilyi
approved these changes
May 31, 2023
Comment on lines
1
to
+12
| import React from 'react'; | ||
| import ReactDOM from 'react-dom/client'; | ||
| import ReactDOM from 'react-dom'; | ||
| import { BrowserRouter, Route } from 'react-router-dom'; | ||
| import Editor from './Editor'; | ||
| import './index.css'; | ||
| import 'higlass/dist/hglib.css'; | ||
|
|
||
| const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement); | ||
| root.render( | ||
| ReactDOM.render( | ||
| <BrowserRouter> | ||
| <Route component={Editor} /> | ||
| </BrowserRouter> | ||
| </BrowserRouter>, | ||
| document.getElementById('root') as HTMLElement |
There was a problem hiding this comment.
Should this also have to be reverted?
There was a problem hiding this comment.
It doesn't have to be, but my thinking was to be consistent in the editor with how we expect most will consume Gosling.js package. When we upgrade react (and it does break in the future), the editor will also break, signaling we need to narrow the react compatibility versions.
In other words, there's no tests in place to ensure gosling-embed.ts works. It would probably be useful to do so, but unifying how we use react in the library in the editor could help surface issues faster.
There was a problem hiding this comment.
Make sense. I am happy with the current version.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes from #898 are breaking to end users who use React 16 or 17 and
gosling.embed(e.g., Gos). The peer-dependencies in Gosling allow forreact-domv16/17/18, but onlyreact-domv18 hasreact-dom/client. The top-levelReactDOM.render()will eventually be deprecated, but it's totally fine to use for the moment.This PR reverts use of
react-dom/client(andcreateRoot), which is specific to react-dom v18, for the backward compatibleReactDOM.render.I'm happy to discuss narrowing support for React versions in Gosling, but the previous changes seemed a bit too restrictive and would likely break many existing Gosling usage for folks who allow for upgrading patches in their dependencies.
Change List
react-dom/clienttoreact-domto avoid breaking change in patch release.Checklist