-
Notifications
You must be signed in to change notification settings - Fork 9
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
use createRoot when possible for rendering pin components (#319)
This PR uses a dynamic imports with a .catch fallback for when react-dom/client does not exist. MapboxMap code had to be tweaked due to differences between ReactDOM.render and ReactDOM.createRoot().render(). Namely, createRoot() does not modify the container element unless it already exists on the physical page. This means that we can't call document.creatElement('div'), use createRoot, and THEN attach it to the mapbox map. Instead, we have to attach the div to the map first. This also seems it make it more difficult to unit test, my guess would be because createRoot does not do anything on the server. For now I use some jest.mocks to ensure the right methods are being called in the right environments. TEST=manual,auto storybook can start up (react 17) test site can display custom pin in both react 17 and 18
- Loading branch information
Showing
5 changed files
with
73 additions
and
5 deletions.
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { ReactElement } from 'react'; | ||
|
||
type Renderer = (reactElement: ReactElement, container: Element) => void; | ||
|
||
// The import must be put into a separate variable, otherwise webpack will | ||
// try to statically resolve the dynamic import and fail to do so | ||
const reactDomClientImportString = 'react-dom/client'; | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore we support both react-dom 17 and 18, but need this | ||
// ts-ignore since react-dom/client does not exist in react-dom 17 | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const reactDomClientPromise: Promise<any> = import(reactDomClientImportString); | ||
|
||
// This can be replaced by a regular import for react-dom/client once we stop supporting react-dom 17 | ||
const rendererPromiseWithFallback: Promise<Renderer> = reactDomClientPromise | ||
.then(reactDomClient => { | ||
const { createRoot } = reactDomClient; | ||
const render: Renderer = (reactElement, container) => { | ||
const root = createRoot(container); | ||
root.render(reactElement); | ||
}; | ||
return render; | ||
}) | ||
.catch(async () => { | ||
const { render } = await import('react-dom'); | ||
return render; | ||
}); | ||
|
||
/** | ||
* Renders the given reactElement into the container. | ||
* Will use createRoot and root.render over ReactDOM.render when possible. | ||
*/ | ||
const renderToContainer = async ( | ||
reactElement: ReactElement, | ||
container: HTMLElement | ||
): Promise<void> => { | ||
const render = await rendererPromiseWithFallback; | ||
await render(reactElement, container); | ||
}; | ||
|
||
export default renderToContainer; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import renderToContainer from '../../../src/components/utils/renderToContainer'; | ||
import packageJson from '../../../package.json'; | ||
|
||
jest.mock('react-dom/client'); | ||
jest.mock('react-dom'); | ||
|
||
/** | ||
* This test needs to pass for all react-dom versions we support. | ||
*/ | ||
it('can render a react component into native dom elements', async () => { | ||
const createRootRender = jest.fn(); | ||
const createRootSpy = jest.spyOn(require('react-dom/client'), 'createRoot') | ||
.mockImplementation(() => ({ render: createRootRender })); | ||
const reactDomRenderSpy = jest.spyOn(require('react-dom'), 'render'); | ||
|
||
const container = document.createElement('div'); | ||
|
||
await renderToContainer(<span>test me</span>, container); | ||
const isReactDom18 = packageJson.devDependencies['react-dom'].match(/18\.\d*\.\d*/); | ||
expect(createRootSpy).toHaveBeenCalledTimes(isReactDom18 ? 1 : 0); | ||
expect(createRootRender).toHaveBeenCalledTimes(isReactDom18 ? 1 : 0); | ||
expect(reactDomRenderSpy).toHaveBeenCalledTimes(isReactDom18 ? 0 : 1); | ||
}); |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
{ | ||
"compilerOptions": { | ||
"module": "esnext", | ||
"strict": true, | ||
"esModuleInterop": true, | ||
"skipLibCheck": true, | ||
|