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.

Sign up for GitHub

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

fix: invalidate module cache on unlinked #2629

Merged
merged 4 commits into from
Mar 29, 2021
Merged

fix: invalidate module cache on unlinked #2629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
938b839
test: add test for file restore hmr
csr632 Mar 21, 2021
5ecb393
fix: invalidate module cache on unlinked
csr632 Mar 21, 2021
ee344b7
fix: fix CI
csr632 Mar 21, 2021
34ce760
fix: remove unused variable
csr632 Mar 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/playground/file-delete-restore/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { useState } from 'react'
import Child from './Child'

function App() {
return (
<div className="App">
<Child />
</div>
)
}

export default App
3 changes: 3 additions & 0 deletions packages/playground/file-delete-restore/Child.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Child() {
return <p>Child state 1</p>
}
61 changes: 61 additions & 0 deletions packages/playground/file-delete-restore/__tests__/file-delete-restore.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import {
editFile,
untilUpdated,
removeFile,
addFile,
isBuild
} from '../../testUtils'

if (!isBuild) {
test('should hmr when file is deleted and restored', async () => {
await untilUpdated(() => page.textContent('p'), 'Child state 1')

editFile('Child.jsx', (code) =>
code.replace('Child state 1', 'Child state 2')
)

await untilUpdated(() => page.textContent('p'), 'Child state 2')

editFile('App.jsx', (code) =>
code
.replace(`import Child from './Child'`, '')
.replace(`<Child />`, '<p>Child deleted</p>')
)
removeFile('Child.jsx')
await untilUpdated(() => page.textContent('p'), 'Child deleted')

// restore Child.jsx
addFile(
'Child.jsx',
` export default function Child() {
return <p>Child state 1</p>
}
`
)

// restore App.jsx
editFile(
'App.jsx',
(code) =>
`import { useState } from 'react'
import Child from './Child'

function App() {
return (
<div className="App">
<Child />
</div>
)
}

export default App
`
)

await untilUpdated(() => page.textContent('p'), 'Child state 1')
})
} else {
test('dummy test to make jest happy', async () => {
// Your test suite must contain at least one test.
})
}
8 changes: 8 additions & 0 deletions packages/playground/file-delete-restore/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div id="app"></div>
<script type="module">
import React from 'react'
import ReactDOM from 'react-dom'
import App from './App.jsx'

ReactDOM.render(React.createElement(App), document.getElementById('app'))
</script>
18 changes: 18 additions & 0 deletions packages/playground/file-delete-restore/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"name": "test-file-delete-restore",
"private": true,
"version": "0.0.0",
"scripts": {
"dev": "vite",
"build": "vite build",
"debug": "node --inspect-brk ../../vite/bin/vite",
"serve": "vite preview"
},
"dependencies": {
"react": "^17.0.1",
"react-dom": "^17.0.1"
},
"devDependencies": {
"@vitejs/plugin-react-refresh": "^1.3.1"
}
}
15 changes: 15 additions & 0 deletions packages/playground/file-delete-restore/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const reactRefresh = require('@vitejs/plugin-react-refresh')

/**
* @type {import('vite').UserConfig}
*/
module.exports = {
plugins: [reactRefresh()],
build: {
// to make tests faster
minify: false
},
esbuild: {
jsxInject: `import React from 'react'`
}
}
18 changes: 9 additions & 9 deletions packages/vite/src/node/server/hmr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,25 +165,25 @@ export async function handleFileAddUnlink(
server: ViteDevServer,
isUnlink = false
) {
const modules = [...(server.moduleGraph.getModulesByFile(file) ?? [])]
if (isUnlink && file in server._globImporters) {
delete server._globImporters[file]
} else {
Copy link
Member

@Shinigami92 Shinigami92 Mar 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you are right with line 179

But we could split this else branch and convert it to an if
So if the first if fulfills, then we don't need to call getModulesByFile to early

if (isUnlink && file in server._globImporters) {
    delete server._globImporters[file]
    return
}

const modules = [...(server.moduleGraph.getModulesByFile(file) ?? [])]
if {
  for (const i in server._globImporters) {
    // ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shinigami92 that would change the logic, in the current state modules returned by getModulesByFile are always updated. Or am I missing something?

Copy link
Member Author

@csr632 csr632 Mar 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current state modules returned by getModulesByFile are always updated.

Agreed. The updateModules should always be called. I think the current logic is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sorry on my side, the code seems really hard to read with all the nested curly braces 🤔
So how else can we achieve it to not call the getModulesByFile to early and safe time that way

Okay, completely new rewrite on my side
What do you think about this one?:

export async function handleFileAddUnlink(
  file: string,
  server: ViteDevServer,
  isUnlink = false
) {
  const needsUnlink = isUnlink && file in server._globImporters
  if (needsUnlink) {
    delete server._globImporters[file]
  }

  const modules = [...(server.moduleGraph.getModulesByFile(file) ?? [])]
  if (!needsUnlink) {
    modules.push(
      ...Object.values(server._globImporters)
        .filter(({ base, pattern }) =>
          match(path.relative(base, file), pattern)
        )
        .map(({ module }) => module)
    )
  }

  if (modules.length > 0) {
    updateModules(
      getShortName(file, server.config.root),
      modules,
      Date.now(),
      server
    )
  }
}

IMO it makes the code a bit more readable and track whats going on

  1. The modules is called as lately as possible
    I know now that it doesn't change runtime performance, but it's a bit more readable that way
  2. Blank spaces make the code / the different if-else branches more readable
  3. The for-in loop now doesn't need an i and collects all needed modules together and then pushes these at once

I don't know if the local variable needsUnlink is the best name here, but from what I read here, I think it is an okay-ish name

Please double check if I didn't broke anything again 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the spacing. About adding needsUnlink to delay the modules init, I would go there when the logic is a bit more complex. I prefer the current branch but I am not opposed to the change.

For the internal for loop, I find the previous one more readable. Maybe a for of could be used if you want to avoid the need for indexing

    for (const { module, base, pattern } of server._globImporters) {
      const relative = path.relative(base, file)
      if (match(relative, pattern)) {
        modules.push(module)
      }
    }

Copy link
Member Author

@csr632 csr632 Mar 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks ok but I don't think such a rewrite is necessary.

The modules is called as lately as possible

I think the two ways have same readability. Declaring variables upfront is a common practice.

Blank spaces make the code / the different if-else branches more readable

I think the if-else is better because it is clear in one sight that only one branch will get executed.

The for-in loop now doesn't need an i and collects all needed modules together and then pushes these at once

I personly prefer the previous way. Maybe the imperative programing style is more intuitive to me.

I want to keep the change minimal so that we don't erase the previous git line blame.

const modules = []
for (const i in server._globImporters) {
const { module, base, pattern } = server._globImporters[i]
const relative = path.relative(base, file)
if (match(relative, pattern)) {
modules.push(module)
}
}
if (modules.length > 0) {
updateModules(
getShortName(file, server.config.root),
modules,
Date.now(),
server
)
}
}
if (modules.length > 0) {
updateModules(
getShortName(file, server.config.root),
modules,
Date.now(),
server
)
}
}

Expand Down