fix(ssr): normalize manifest filenames

[ferdinando-ferreira]

Description

Fixed #3303

As described on #3303, both ssr-manifest.json and the ssrContext embedded in the resulting server entry contains information about the filenames present in the source code of the compilation. As requested on #3315 only the normalization part is addressed by this PR.

Additional context

It is possible to reproduce using the ssr-vue own example in this codebase.

git clone https://github.com/vitejs/vite.git vite
cd vite
yarn
yarn build
cd packages/playground/ssr-vue
yarn build

Two files will contain complete paths from the source code:

• packages/playground/ssr-vue/dist/client/ssr-manifest.json
• packages/playground/ssr-vue/dist/server/entry-server.js

This PR changes the generation of both the manifest and the ssrContext by using, instead of the complete filenames, their filenames relative to options.root, to ensure compilations of the same codebase in different tree structures will result in the same hashes, ensuring determinism

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[whaijungit]

vite.config.js server config proxy invalid

export defualt{
   server:{
      proxy:{
        "/api":{
         target:"http://localhost:8000",
         changeOrigin:true,
         rewrite: (path) => path.replace(/^\/api/, '')
        }
      }
   }
}

browser result main.js:5 GET http://192.168.28.99:3000/api/article/1231 404 (Not Found)

why ??

[brillout]

@Shinigami92 Is there anything blocking this from getting merged?

[brillout]

@whaijungit I'm guessing your comment to be unrelated to this PR. You can try to get help on Discord.

[whaijungit]

OK, thank you、There are too many holes in vite 、bet http-proxy-middlewear

[patak-dev]

@ferdinando-ferreira looks like the paths are not normalized in Windows. Here is a gist with the manifest for playground/ssr-vue https://gist.github.com/patak-js/4b903cccc02ed55d60b43cc2a305fa3c
It should be a matter of using normalizePath to get posix paths in all systems.
Do you think is possible to add a test for the manifest so we check this in CI?

[ferdinando-ferreira]

It should be a matter of using normalizePath to get posix paths in all systems.

@patak-js : Done.

Notice that @rollup/pluginutils normalizePath was used instead of the one on vite own utils because the former is available at zero cost for plugins while the later would require requiring vite' into the plugin.

Question for a potential other PR: is there a reason for the existence of vite's own normalizePath implementation instead of using the one coming from rollup/pluginutils?

Do you think is possible to add a test for the manifest so we check this in CI?

It is possible but I am not familiar of the way this project prefers it to be done. I can suggest a test to be added to ssr-vue on the playground, with toMatchSnapshot between the generated ssr-manifest.json and the snapshot.

Given that this whole PR purpose is to ensure any build of the same code will generate the same manifest using a snapshot could be fitting.

Would that be appropriate?

[ferdinando-ferreira]

#3865 fixes the regression that caused the Windows build CI to fail.

[patak-dev]

It should be a matter of using normalizePath to get posix paths in all systems.

@patak-js : Done.

Notice that @rollup/pluginutils normalizePath was used instead of the one on vite own utils because the former is available at zero cost for plugins while the later would require requiring vite' into the plugin.

Question for a potential other PR: is there a reason for the existence of vite's own normalizePath implementation instead of using the one coming from rollup/pluginutils?

In rollup, it is just replacing the win separator by posix ones:

const normalizePath: NormalizePath = function normalizePath(filename: string) {
  return filename.split(win32.sep).join(posix.sep);
};

In vite we also have a call to https://nodejs.org/api/path.html#path_path_normalize_path

The path.normalize() method normalizes the given path, resolving '..' and '.' segments.

export function normalizePath(id: string): string {
  return path.posix.normalize(isWindows ? slash(id) : id)
}

I also think that this is not ideal. I don't know how to solve this though, maybe the maintainers of rollup are open to also add path.posix.normalize call (although it may break things there). Or we should avoid it, and see where in our codebase this extra normalization is really needed.

Do you think is possible to add a test for the manifest so we check this in CI?

It is possible but I am not familiar of the way this project prefers it to be done. I can suggest a test to be added to ssr-vue on the playground, with toMatchSnapshot between the generated ssr-manifest.json and the snapshot.

Given that this whole PR purpose is to ensure any build of the same code will generate the same manifest using a snapshot could be fitting.

Would that be appropriate?

We are only using toMatchInlineSnapshot at this point in some internal ssr tests. But I think this would be ok in this case, if there is a comment explaining how to update in case is needed. @antfu what do you think?

[ferdinando-ferreira]

In vite we also have a call to https://nodejs.org/api/path.html#path_path_normalize_path

In this particular PR the paths all start absolute and then are turned to relative with path.relative. Replacing the path separator is enough to satisfy the requirement.

As for the test I'll wait for @antfu opinion but, given the limited scope of this PR, maybe it would be fine merging it and creating a test for it in a separate PR.