From d5e56a1267401dec4be58ffd9c59d3e58c84b7e7 Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Sat, 24 Sep 2022 22:26:42 -0400 Subject: [PATCH 1/8] test: add failing test --- playground/react/App.jsx | 4 +++- playground/react/__tests__/react.spec.ts | 15 ++++++++++++++- playground/react/hmr/no-exported-comp.jsx | 7 +++++++ playground/react/hmr/parent.jsx | 7 +++++++ 4 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 playground/react/hmr/no-exported-comp.jsx create mode 100644 playground/react/hmr/parent.jsx diff --git a/playground/react/App.jsx b/playground/react/App.jsx index 3ec29ba38d893b..f9ca273260c08c 100644 --- a/playground/react/App.jsx +++ b/playground/react/App.jsx @@ -1,6 +1,7 @@ import { useState } from 'react' -import Dummy from './components/Dummy?qs-should-not-break-plugin-react' import Button from 'jsx-entry' +import Dummy from './components/Dummy?qs-should-not-break-plugin-react' +import Parent from './hmr/parent' function App() { const [count, setCount] = useState(0) @@ -27,6 +28,7 @@ function App() { + ) diff --git a/playground/react/__tests__/react.spec.ts b/playground/react/__tests__/react.spec.ts index 15f6319220d7f2..1911ad7558807a 100644 --- a/playground/react/__tests__/react.spec.ts +++ b/playground/react/__tests__/react.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from 'vitest' -import { editFile, isServe, page, untilUpdated } from '~utils' +import { browserLogs, editFile, isServe, page, untilUpdated } from '~utils' test('should render', async () => { expect(await page.textContent('h1')).toMatch('Hello Vite + React') @@ -18,6 +18,19 @@ test('should hmr', async () => { expect(await page.textContent('button')).toMatch('count is: 1') }) +// #9869 +test('should only hmr files with exported react components', async () => { + browserLogs.length = 0 + editFile('hmr/no-exported-comp.jsx', (code) => + code.replace('An Object', 'Updated') + ) + await untilUpdated(() => page.textContent('#parent'), 'Updated') + expect(browserLogs).toMatchObject([ + '[vite] hot updated: /hmr/parent.jsx', + 'Parent rendered' + ]) +}) + test.runIf(isServe)( 'should have annotated jsx with file location metadata', async () => { diff --git a/playground/react/hmr/no-exported-comp.jsx b/playground/react/hmr/no-exported-comp.jsx new file mode 100644 index 00000000000000..7784bcb50603a9 --- /dev/null +++ b/playground/react/hmr/no-exported-comp.jsx @@ -0,0 +1,7 @@ +// This un-exported react component should not cause this file to be treated +// as an HMR boundary +const Unused = () => An unused react component + +export const Foo = { + is: 'An Object' +} diff --git a/playground/react/hmr/parent.jsx b/playground/react/hmr/parent.jsx new file mode 100644 index 00000000000000..ff8698281c83c7 --- /dev/null +++ b/playground/react/hmr/parent.jsx @@ -0,0 +1,7 @@ +import { Foo } from './no-exported-comp' + +export default function Parent() { + console.log('Parent rendered') + + return
{Foo.is}
+} From 46971d30da10122d72fdb5765addc5e4c252c6b1 Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Sun, 25 Sep 2022 01:03:16 -0400 Subject: [PATCH 2/8] fix: conditionally accept if all exports are react components But this is broken because Vite still treats this as self-accepting. --- packages/plugin-react/src/fast-refresh.ts | 53 +++++++++++++++++++---- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/packages/plugin-react/src/fast-refresh.ts b/packages/plugin-react/src/fast-refresh.ts index b3b095a65cf2ae..7ca3b1af04217c 100644 --- a/packages/plugin-react/src/fast-refresh.ts +++ b/packages/plugin-react/src/fast-refresh.ts @@ -58,20 +58,56 @@ if (import.meta.hot) { window.$RefreshSig$ = RefreshRuntime.createSignatureFunctionForTransform; }`.replace(/[\n]+/gm, '') -const footer = ` -if (import.meta.hot) { - window.$RefreshReg$ = prevRefreshReg; - window.$RefreshSig$ = prevRefreshSig; - - __ACCEPT__ +const timeout = ` if (!window.__vite_plugin_react_timeout) { window.__vite_plugin_react_timeout = setTimeout(() => { window.__vite_plugin_react_timeout = 0; RefreshRuntime.performReactRefresh(); }, 30); } +` + +const footer = ` +if (import.meta.hot) { + window.$RefreshReg$ = prevRefreshReg; + window.$RefreshSig$ = prevRefreshSig; + + __ACCEPT__ }` +const checkAndAccept = ` +function isReactRefreshBoundary(mod) { + if (mod == null || typeof mod !== 'object') { + return false; + } + let hasExports = false; + let areAllExportsComponents = true; + for (const exportName in mod) { + hasExports = true; + if (exportName === '__esModule') { + continue; + } + const desc = Object.getOwnPropertyDescriptor(mod, exportName); + if (desc && desc.get) { + // Don't invoke getters as they may have side effects. + return false; + } + const exportValue = mod[exportName]; + if (!RefreshRuntime.isLikelyComponentType(exportValue)) { + areAllExportsComponents = false; + } + } + return hasExports && areAllExportsComponents; +} + +import(/* @vite-ignore */ import.meta.url).then(mod => { + if (isReactRefreshBoundary(mod)) { + import.meta.hot.accept(); + ${timeout} + } +}) +` + export function addRefreshWrapper( code: string, id: string, @@ -80,12 +116,13 @@ export function addRefreshWrapper( return ( header.replace('__SOURCE__', JSON.stringify(id)) + code + - footer.replace('__ACCEPT__', accept ? 'import.meta.hot.accept();' : '') + footer.replace('__ACCEPT__', accept ? checkAndAccept : timeout) ) } export function isRefreshBoundary(ast: t.File): boolean { - // Every export must be a React component. + // Every export must be a potential React component. + // We'll also perform a runtime check that's more robust as well (isLikelyComponentType). return ast.program.body.every((node) => { if (node.type !== 'ExportNamedDeclaration') { return true From 2c26991cf69faf2eb40c98d32b8cf286e29c2062 Mon Sep 17 00:00:00 2001 From: Alec Larson <1925840+aleclarson@users.noreply.github.com> Date: Sun, 25 Sep 2022 13:36:46 -0400 Subject: [PATCH 3/8] fix: always call `hot.accept` --- packages/plugin-react/src/fast-refresh.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin-react/src/fast-refresh.ts b/packages/plugin-react/src/fast-refresh.ts index 7ca3b1af04217c..f577af3f3fcebc 100644 --- a/packages/plugin-react/src/fast-refresh.ts +++ b/packages/plugin-react/src/fast-refresh.ts @@ -101,8 +101,8 @@ function isReactRefreshBoundary(mod) { } import(/* @vite-ignore */ import.meta.url).then(mod => { + import.meta.hot.accept(); if (isReactRefreshBoundary(mod)) { - import.meta.hot.accept(); ${timeout} } }) From ce093e1d47d9b652e66c1a33c844bdf8bdc141b7 Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Sun, 25 Sep 2022 21:42:54 -0400 Subject: [PATCH 4/8] fix: invalidate hmr if not boundary --- packages/plugin-react/src/fast-refresh.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/plugin-react/src/fast-refresh.ts b/packages/plugin-react/src/fast-refresh.ts index f577af3f3fcebc..ae55bd2a97bcfa 100644 --- a/packages/plugin-react/src/fast-refresh.ts +++ b/packages/plugin-react/src/fast-refresh.ts @@ -104,6 +104,8 @@ import(/* @vite-ignore */ import.meta.url).then(mod => { import.meta.hot.accept(); if (isReactRefreshBoundary(mod)) { ${timeout} + } else { + import.meta.hot.invalidate(); } }) ` From 308421fd1778e00c2ae5b8365da90a45d72d29ee Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Sun, 25 Sep 2022 21:49:09 -0400 Subject: [PATCH 5/8] test: fix test expectation --- playground/react/__tests__/react.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/playground/react/__tests__/react.spec.ts b/playground/react/__tests__/react.spec.ts index 1911ad7558807a..cfbbf9962419dd 100644 --- a/playground/react/__tests__/react.spec.ts +++ b/playground/react/__tests__/react.spec.ts @@ -26,9 +26,11 @@ test('should only hmr files with exported react components', async () => { ) await untilUpdated(() => page.textContent('#parent'), 'Updated') expect(browserLogs).toMatchObject([ + '[vite] hot updated: /hmr/no-exported-comp.jsx', '[vite] hot updated: /hmr/parent.jsx', 'Parent rendered' ]) + browserLogs.length = 0 }) test.runIf(isServe)( From 7d5aa5db86239c22c4ef972528aa72314773ccf4 Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Sun, 25 Sep 2022 21:49:30 -0400 Subject: [PATCH 6/8] test: add test for react context hmr --- playground/react/App.jsx | 20 +++++++++++-- playground/react/__tests__/react.spec.ts | 35 ++++++++++++++++++---- playground/react/context/ContextButton.jsx | 11 +++++++ playground/react/context/CountProvider.jsx | 12 ++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 playground/react/context/ContextButton.jsx create mode 100644 playground/react/context/CountProvider.jsx diff --git a/playground/react/App.jsx b/playground/react/App.jsx index f9ca273260c08c..83f4cc07ea4a07 100644 --- a/playground/react/App.jsx +++ b/playground/react/App.jsx @@ -2,6 +2,8 @@ import { useState } from 'react' import Button from 'jsx-entry' import Dummy from './components/Dummy?qs-should-not-break-plugin-react' import Parent from './hmr/parent' +import { CountProvider } from './context/CountProvider' +import { ContextButton } from './context/ContextButton' function App() { const [count, setCount] = useState(0) @@ -10,10 +12,16 @@ function App() {

Hello Vite + React

-

+

+ +

Edit App.jsx and save to test HMR updates.

@@ -34,4 +42,12 @@ function App() { ) } -export default App +function AppWithProviders() { + return ( + + + + ) +} + +export default AppWithProviders diff --git a/playground/react/__tests__/react.spec.ts b/playground/react/__tests__/react.spec.ts index cfbbf9962419dd..9f170cb13df17c 100644 --- a/playground/react/__tests__/react.spec.ts +++ b/playground/react/__tests__/react.spec.ts @@ -6,16 +6,16 @@ test('should render', async () => { }) test('should update', async () => { - expect(await page.textContent('button')).toMatch('count is: 0') - await page.click('button') - expect(await page.textContent('button')).toMatch('count is: 1') + expect(await page.textContent('#state-button')).toMatch('count is: 0') + await page.click('#state-button') + expect(await page.textContent('#state-button')).toMatch('count is: 1') }) test('should hmr', async () => { editFile('App.jsx', (code) => code.replace('Vite + React', 'Updated')) await untilUpdated(() => page.textContent('h1'), 'Hello Updated') // preserve state - expect(await page.textContent('button')).toMatch('count is: 1') + expect(await page.textContent('#state-button')).toMatch('count is: 1') }) // #9869 @@ -37,7 +37,7 @@ test.runIf(isServe)( 'should have annotated jsx with file location metadata', async () => { const meta = await page.evaluate(() => { - const button = document.querySelector('button') + const button = document.querySelector('#state-button') const key = Object.keys(button).find( (key) => key.indexOf('__reactFiber') === 0 ) @@ -52,3 +52,28 @@ test.runIf(isServe)( ]) } ) + +test('should hmr react context', async () => { + browserLogs.length = 0 + expect(await page.textContent('#context-button')).toMatch( + 'context-based count is: 0' + ) + await page.click('#context-button') + expect(await page.textContent('#context-button')).toMatch( + 'context-based count is: 1' + ) + editFile('context/CountProvider.jsx', (code) => + code.replace('context provider', 'context provider updated') + ) + await untilUpdated( + () => page.textContent('#context-provider'), + 'context provider updated' + ) + expect(browserLogs).toMatchObject([ + '[vite] hot updated: /context/CountProvider.jsx', + '[vite] hot updated: /App.jsx', + '[vite] hot updated: /context/ContextButton.jsx', + 'Parent rendered' + ]) + browserLogs.length = 0 +}) diff --git a/playground/react/context/ContextButton.jsx b/playground/react/context/ContextButton.jsx new file mode 100644 index 00000000000000..92c6d0bd26f968 --- /dev/null +++ b/playground/react/context/ContextButton.jsx @@ -0,0 +1,11 @@ +import { useContext } from 'react' +import { CountContext } from './CountProvider' + +export function ContextButton() { + const { count, setCount } = useContext(CountContext) + return ( + + ) +} diff --git a/playground/react/context/CountProvider.jsx b/playground/react/context/CountProvider.jsx new file mode 100644 index 00000000000000..223ad25f04f056 --- /dev/null +++ b/playground/react/context/CountProvider.jsx @@ -0,0 +1,12 @@ +import { createContext, useState } from 'react' +export const CountContext = createContext() + +export const CountProvider = ({ children }) => { + const [count, setCount] = useState(0) + return ( + + {children} +
context provider
+ + ) +} From f2ec7b1d40742cd366d155e6862953a37d73827c Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Sun, 25 Sep 2022 22:00:44 -0400 Subject: [PATCH 7/8] test: only run hmr tests in serve --- playground/react/__tests__/react.spec.ts | 90 +++++++++++++----------- 1 file changed, 50 insertions(+), 40 deletions(-) diff --git a/playground/react/__tests__/react.spec.ts b/playground/react/__tests__/react.spec.ts index 9f170cb13df17c..654a45a668e894 100644 --- a/playground/react/__tests__/react.spec.ts +++ b/playground/react/__tests__/react.spec.ts @@ -1,5 +1,12 @@ import { expect, test } from 'vitest' -import { browserLogs, editFile, isServe, page, untilUpdated } from '~utils' +import { + browserLogs, + editFile, + isBuild, + isServe, + page, + untilUpdated +} from '~utils' test('should render', async () => { expect(await page.textContent('h1')).toMatch('Hello Vite + React') @@ -18,21 +25,6 @@ test('should hmr', async () => { expect(await page.textContent('#state-button')).toMatch('count is: 1') }) -// #9869 -test('should only hmr files with exported react components', async () => { - browserLogs.length = 0 - editFile('hmr/no-exported-comp.jsx', (code) => - code.replace('An Object', 'Updated') - ) - await untilUpdated(() => page.textContent('#parent'), 'Updated') - expect(browserLogs).toMatchObject([ - '[vite] hot updated: /hmr/no-exported-comp.jsx', - '[vite] hot updated: /hmr/parent.jsx', - 'Parent rendered' - ]) - browserLogs.length = 0 -}) - test.runIf(isServe)( 'should have annotated jsx with file location metadata', async () => { @@ -53,27 +45,45 @@ test.runIf(isServe)( } ) -test('should hmr react context', async () => { - browserLogs.length = 0 - expect(await page.textContent('#context-button')).toMatch( - 'context-based count is: 0' - ) - await page.click('#context-button') - expect(await page.textContent('#context-button')).toMatch( - 'context-based count is: 1' - ) - editFile('context/CountProvider.jsx', (code) => - code.replace('context provider', 'context provider updated') - ) - await untilUpdated( - () => page.textContent('#context-provider'), - 'context provider updated' - ) - expect(browserLogs).toMatchObject([ - '[vite] hot updated: /context/CountProvider.jsx', - '[vite] hot updated: /App.jsx', - '[vite] hot updated: /context/ContextButton.jsx', - 'Parent rendered' - ]) - browserLogs.length = 0 -}) +if (!isBuild) { + // #9869 + test('should only hmr files with exported react components', async () => { + browserLogs.length = 0 + editFile('hmr/no-exported-comp.jsx', (code) => + code.replace('An Object', 'Updated') + ) + await untilUpdated(() => page.textContent('#parent'), 'Updated') + expect(browserLogs).toMatchObject([ + '[vite] hot updated: /hmr/no-exported-comp.jsx', + '[vite] hot updated: /hmr/parent.jsx', + 'Parent rendered' + ]) + browserLogs.length = 0 + }) + + // #3301 + test('should hmr react context', async () => { + browserLogs.length = 0 + expect(await page.textContent('#context-button')).toMatch( + 'context-based count is: 0' + ) + await page.click('#context-button') + expect(await page.textContent('#context-button')).toMatch( + 'context-based count is: 1' + ) + editFile('context/CountProvider.jsx', (code) => + code.replace('context provider', 'context provider updated') + ) + await untilUpdated( + () => page.textContent('#context-provider'), + 'context provider updated' + ) + expect(browserLogs).toMatchObject([ + '[vite] hot updated: /context/CountProvider.jsx', + '[vite] hot updated: /App.jsx', + '[vite] hot updated: /context/ContextButton.jsx', + 'Parent rendered' + ]) + browserLogs.length = 0 + }) +} From 535aa4f75b114bfab0bc8f67369b6560df485a82 Mon Sep 17 00:00:00 2001 From: Ian VanSchooten Date: Mon, 26 Sep 2022 09:49:41 -0400 Subject: [PATCH 8/8] fix: move invalidate into accept callback --- packages/plugin-react/src/fast-refresh.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/plugin-react/src/fast-refresh.ts b/packages/plugin-react/src/fast-refresh.ts index ae55bd2a97bcfa..b0b38a8cafb94e 100644 --- a/packages/plugin-react/src/fast-refresh.ts +++ b/packages/plugin-react/src/fast-refresh.ts @@ -100,14 +100,13 @@ function isReactRefreshBoundary(mod) { return hasExports && areAllExportsComponents; } -import(/* @vite-ignore */ import.meta.url).then(mod => { - import.meta.hot.accept(); +import.meta.hot.accept(mod => { if (isReactRefreshBoundary(mod)) { ${timeout} } else { import.meta.hot.invalidate(); } -}) +}); ` export function addRefreshWrapper(