Support for moduleInfo.meta in resolveId hook still not equivalent in vite dev and vite build

[joaotavora]

Describe the bug

In #5465 support for moduleInfo.meta is added to the dev server. This is a very welcome change that I look forward to using. Unforunately, behavioral parity between vite build and vite dev is not quite there yet. In the attached reproduction recipe the former succeeds while the latter fails. By "failing" I mean the transform hook is never called and doesn't inject an important transformation into foo.js. Here are relevant parts of the files.

//main.js
import fn from './foo.js?xoption=21'
document.querySelector('#app').innerHTML = `<h1>Hello Vite: ${fn()}</h1>`

//foo.js
export default function () {return 21 + window.THE_X;}

//vite.config.js
import path from 'path';
export default {
    plugins: [
        {
            enforce: 'pre',
            async resolveId(id) {
                let [file, query] = id.split('?');
                const probe = query && query.match(/xoption=([^&]+)/)
                const x = probe && Number(probe[1])
                if (x) return {id: path.resolve(file), meta: {x}}
            },
            async transform(blob, id) {
                const x = this.getModuleInfo(id)?.meta?.x
                if (x) return `window.THE_X = ${x}\n` + blob;
            }
        }
    ]
}

To be clear, this particular solution "never" worked with Vite before. However, before #5465, a workaround/hack (not shown) could be devised, because getModuleInfo called in he resolveId hook would return a persistent empty object that could be used to fill in the shortcomings that #5465 hopes to address. Ironically, that workaround is now not possible anymore.

If this is accepted as a bug, I'd be happy to provide a PR to fix this.

Reproduction

https://github.com/joaotavora/bug-vite-return-meta-from-resolve-id

System Info

System:
    OS: Linux 5.15 Arch Linux
    CPU: (4) x64 Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
    Memory: 3.21 GB / 15.39 GB
    Container: Yes
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.9.0 - ~/.nvm/versions/node/v16.9.0/bin/node
    Yarn: 1.22.11 - ~/.nvm/versions/node/v16.9.0/bin/yarn
    npm: 7.21.1 - ~/.nvm/versions/node/v16.9.0/bin/npm
  Browsers:
    Chromium: 96.0.4664.45
  npmPackages:
    vite: ^2.7.2 => 2.8.0-beta.5

Used Package Manager

npm

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 https://github.com/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.

[aleclarson]

It's your own bug :)

The id property you return from resolveId is not a real file. You have to remove the leading slash before passing file to path.resolve or else Vite won't be able to load the file from disk.

[joaotavora]

The id property you return from resolveId is not a real file.

You're misreading the code. Did you try the minimal reproducible repo I constructed and attached?

Here is a change dversion of vite.config.js that prints out exactly the file being returned from the resolveId hook. It always exists. It's easy to check, just npx vite dev -d with the repro repo I sent you.

import path from 'path';
export default {
    plugins: [
        {
            enforce: 'pre',
            async resolveId(id) {
                let [file, query] = id.split('?');
                const probe = query && query.match(/xoption=([^&]+)/)
                const x = probe && Number(probe[1])
                if (x) {
                    const realone = path.resolve(file);
                    console.log('IS THIS FILE REAL ON THE FS?', realone);
                    return {id: realone, meta: {x}}
                }
            },
            async transform(blob, id) {
                const x = this.getModuleInfo(id)?.meta?.x
                if (x) return `window.THE_X = ${x}\n` + blob;
            }
        }
    ]
}  

logs

vite:load 10.95ms [fs] /main.js +9ms
IS THIS FILE REAL ON THE FS? /home/capitaomorte/Source/Javascript/bug-vite-return-meta-from-resolve-id/foo.js
vite:resolve 0.22ms ./foo.js?xoption=21 -> /home/capitaomorte/Source/Javascript/bug-vite-return-meta-from-resolve-id/foo.js +6ms
vite:resolve 0.15ms /foo.js -> /home/capitaomorte/Source/Javascript/bug-vite-return-meta-from-resolve-id/foo.js +0ms
vite:import-analysis 2.53ms [1 imports rewritten] main.js +6ms

Also, why would this work with npx vite build?

[joaotavora]

Hi @aleclarson. I hope you can appreciate this report again. I've dug down to the root of the problem. Here is a possible fix (but not the cleanest one, I'll present that cleaner one later as a PR).

The comments should hint as to what goes wrong and how this fixes it.

diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts
index 953304e0..4a5c5281 100644
--- a/packages/vite/src/node/plugins/importAnalysis.ts
+++ b/packages/vite/src/node/plugins/importAnalysis.ts
@@ -178,6 +178,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
         url: string,
         pos: number
       ): Promise<[string, string]> => {
+        const uncleanUrl = url;
         if (base !== '/' && url.startsWith(base)) {
           url = url.replace(base, '/')
         }
@@ -259,7 +260,9 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
           // its last updated timestamp to force the browser to fetch the most
           // up-to-date version of this module.
           try {
-            const depModule = await moduleGraph.ensureEntryFromUrl(url, ssr)
+            // make sure we feed the original "unclean" URL to  ensureEntryFromUrl
+            // so as to call the very same user plugins as in build mode.
+            const depModule = await moduleGraph.ensureEntryFromUrl(uncleanUrl, ssr)
             if (depModule.lastHMRTimestamp > 0) {
               url = injectQuery(url, `t=${depModule.lastHMRTimestamp}`)
             }
diff --git a/packages/vite/src/node/server/moduleGraph.ts b/packages/vite/src/node/server/moduleGraph.ts
index f09e741f..75d0bcf7 100644
--- a/packages/vite/src/node/server/moduleGraph.ts
+++ b/packages/vite/src/node/server/moduleGraph.ts
@@ -160,7 +160,10 @@ export class ModuleGraph {
     let mod = this.urlToModuleMap.get(url)
     if (!mod) {
       mod = new ModuleNode(url)
-      if (meta) mod.meta = meta
+      // A module with the same 'resolveId' might already exist under
+      // a different URL. Make sure we inherit its 'meta'.
+      const existing = this.idToModuleMap.get(resolvedId)
+      mod.meta = {...existing?.meta, ...meta}
       this.urlToModuleMap.set(url, mod)
       mod.id = resolvedId
       this.idToModuleMap.set(resolvedId, mod)

I'm not a fan of this fix, but it does fix it and hopefully illustrate the problem. It will fail if the moduleGraph.ensureEntryFromUrl is ever moved outside normalizeUrl.

[aleclarson]

Did you try the minimal reproducible repo I constructed and attached?

I tested it in a Github codespace. I believe this will fix your issue:

diff --git a/plugin.js b/plugin.js
index 20ac4f7..c4f07c2 100644
--- a/plugin.js
+++ b/plugin.js
@@ -4,11 +4,14 @@ export default {
     plugins: [
         {
             enforce: 'pre',
-            async resolveId(id) {
+            async resolveId(id, importer) {
                 let [file, query] = id.split('?');
-                const probe = query && query.match(/xoption=([^&]+)/)
-                const x = probe && Number(probe[1])
-                if (x) return {id: path.resolve(file), meta: {x}}
+                const probe = query && query.match(/xoption=([^&]+)/);
+                const x = probe && Number(probe[1]);
+                if (isNaN(x)) return;
+                const resolved = await this.resolve(file, importer, {skipSelf: true});
+                if (!resolved) return;
+                return {...resolved, meta: { ...resolved.meta, x }};
             },
             async transform(blob, id) {
                 const x = this.getModuleInfo(id)?.meta?.x

[joaotavora]

On Sun, Feb 6, 2022 at 6:47 PM Alec Larson ***@***.***> wrote: Did you try the minimal reproducible repo I constructed and attached? I did, and my suggestion fixed it

OK. There must be some misunderstanding. The sentence you wrote on Sat, 5 Feb 2022 22:31:40 -0800 (PST) goes

> The id property you return from resolveId is not a real file

But that is just not true. When my resolveId returns {id}, it is indeed a real file. I've added an assertion to prove it. This plugin works with "bare" non-Vite Rollup and also works with `vite build`, which is mostly bare Rollup. When either of the two suggestion commits are applied to Vite, then this plugin works all around.

[joaotavora]

@aleclarson the minimal reproduction recipe I attached is just a proof of the bug.

Yes, know I can use this.resolve to work around it (you seem to have
abandonedyour earlier "just remove the leading slash" idea, which is good).

But applying the this.resolve complexity to work around much larger
problem I have in my codebase (which is not the MRE, of course) , doesn't
change the fact that vite dev behaves differently from vite build.

The bug in Vite is fixed by either one of those two commits (the first one
being preferable). Of course there are multiple ways to skin a cat, so it
you come up with a third fix, that's fine too!

Thanks for your time and work.
João

[joaotavora]

Just wanted to add that the solution that I recommend,
joaotavora@6e6a27f
needs a small refactor commit that immediately precedes it.

I hope you can consider what is a very small fix to a blind spot
of #5465 (which, as I've said, is a very welcome fix otherwise).
As far as I can tell my commits don't break any tests and it's
easy to tell what's going on from the diff.

[aleclarson]

Please open a PR and we can get Evan's feedback

[joaotavora]

Please open a PR and we can get Evan's feedback

Done. See. #6811

Shouldn't this issue be reopened so that that that PR (or any other PR...) can subsequentely close it?

[joaotavora]

@patak-dev Just noticed there's a "rollup plugin compat" label, maybe add it here?