fix: invalidate module cache on unlinked

[csr632]

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

---

Description

Fix #2630

When a file is deleted and restored, previously vite keeps using cache with out-of-date content(content before file is deleted), intead of the restored content.

This PR make sure module cache is invalidated when file is unlinked.

Additional context

[Shinigami92]

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) {
    // ...

[patak-dev]

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

[csr632]

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.

[Shinigami92]

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 😅

[patak-dev]

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)
      }
    }

[csr632]

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.