a script tag referencing node_modules does not get browserHash injected causing double-instantiation

[Rich-Harris]

Describe the bug

I'm not sure if this is a bug or expected behaviour, but it's left me scratching my head and I'd love to know if there's a workaround at the very least.

When a module is imported via a project-relative URL (/node_modules/foo/index.js) and via a package name (foo), Vite resolves the latter to /node_modules/foo/index.js?v=xyz123, which causes the module to be instantiated twice. (In this example, optimizeDeps.exclude = ['foo'].)

This is causing headaches for SvelteKit users, as it causes failed instanceof checks. It doesn't happen when the package in question is installed outside the directory (e.g. in a workspace root), because in that case both references are resolved to /@fs/path/to/project/node_modules/foo/index.js.

Reproduction

https://github.com/Rich-Harris/vite-browserhash-repro

System Info

System:
    OS: macOS 12.5.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 178.69 MB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.15.1 - ~/.nvm/versions/node/v16.15.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.15.1/bin/yarn
    npm: 8.11.0 - ~/.nvm/versions/node/v16.15.1/bin/npm
  Browsers:
    Chrome: 104.0.5112.101
    Chrome Canary: 107.0.5258.0
    Firefox: 103.0.2
    Safari: 15.6.1
  npmPackages:
    vite: ^3.0.9 => 3.0.9

Used Package Manager

pnpm

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[benmccann]

Related issue, we may have a regression here #9661. We need to hash files (with ?v= to be able to reload them after they have been re-optimized.

For the example that Rich added, if 'foo' is in optimizeDeps exclude, it may still import other dependencies (that have been optimized) so we still need to have a ?v= on these excluded files.

[patak-dev]

I'm not sure if this is a bug or expected behaviour

It is a bug, for reference, I added an explanation of why we need version queries in direct node_modules imports in the description of:

• fix: ensure version query for direct node_modules imports #9848

[sapphi-red]

Sveltekit was fixed by #9848 but the repro in the OP is not fixed. This is happening because we are not injecting browserHash to script tags referencing a file in node_modules. (https://discord.com/channels/804011606160703521/804439875226173480/1172385782007353344)

A workaround is to use <script type="module">import 'path/to/node_module/file'</script> instead of <script type="module" src="path/to/node_module/file"></script>.

I guess this would also be fixed if we make bare specifiers work in HTML in dev (#12260). Though I'm not sure if we are going to support that.