fix: fix rollup plugin compatibility when getModuleInfo() is used

[joaotavora]

Description

Hello. This PR addresses and fixes #6766. @aleclarson requested I present it. In the issue, I show how a Rollup plugin such as this:

//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;
            }
        }
    ]
}

... exhibits different behaviour when exercised via vite build and vite dev. The two small files main.js and foo.js can help understand the context.

//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;}

Additional context

This change improves the parity of behaviour between vite build and vite dev when Rollup plugins retuning non-null meta properties from resolveId() hooks are used.

If one takes the Rollup behavior as reference in this case (I think it's well inside of this criteria) then this is fixing a small bug in Vite.

That bug was in a very large part already fixed by PR #5465 which @aleclarson authored. Upgrading to a recent Vite version containing the PR is my prime motivation for this PR.

Here's an explanation of the faulty behaviour:

The normalizeUrl() helper calls user plugins's resolveId() twice.

• Once with the URL string containing a query suffix. Here it ignores the meta reply.

• Again, with the URL no longer containing said suffix. Here it doesn't ignore the meta reply. It is stored in the moduleGraph node.

As can be seen, if the user's plugin meta reply depends on the query suffix being passed in the imported URL, that meta reply won't be registered correctly in the module graph and is not retrievable via getModuleInfo() later on.

This change makes it so that the first call doesn't ignore the meta reply. This makes Vite's dev server match Rollup's plugin-calling behaviour and fixes #6766.

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.

[joaotavora]

It the general direction of the PR is agreeable, I can probably include some automatic tests that fail without this PR and pass with it.

[joaotavora]

I've converted this into a draft as I've noticed that there's still an example where vite build works and vite dev doesn't. It's even simpler:

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

//vite.config.js
const virtualId = "@foo.js"
import path from 'path';
export default {
  plugins: [
    {
      enforce: 'pre',
      async resolveId(id) {
        let [prequery, query] = id.split('?');
        if (prequery !== virtualId) return;
        const probe = query && query.match(/xoption=([^&]+)/)
        const x = probe && Number(probe[1])
        if (x) {
          return {id: prequery, meta: {x}}
        }
      },
      async load(id) {
        if (id === virtualId) {
          const x = this.getModuleInfo(id)?.meta?.x
          if (!x) throw new Error('OOPS')
          return `export default function () {return 21 + ${x};}`
        }
      }
    }
  ]
}

vite dev throws, vite build doesn't

[joaotavora]

I've converted this into a draft as I've noticed that there's still an example where vite build works and vite dev doesn't. It's even simpler:

I've fixed this second problem.

I've also added an automated test that fails without this PR and passes with it. Rebased the PR so that the failing test comes first, then three commits which fix the failing test.

Ping @patak-dev @aleclarson, please have a look at this PR.

[aleclarson]

Can you make a Stackblitz for each of the failing plugins? You can start from https://vite.new/react

[joaotavora]

Can you make a Stackblitz for each of the failing plugins? You can start from https://vite.new/react

https://stackblitz.com/edit/vitejs-vite-rgteb2?file=vite.config.js

https://stackblitz.com/edit/vitejs-vite-vf7nho?file=vite.config.js

[joaotavora]

Friendly ping. Any news here? Should I also make a Rollup "repl.it" recipe to show that this behaves nicely on Rollup?

[aleclarson]

Why did I change this? This was the final spot where the resolved metadata was getting lost, so I've reverted the change you made inside the ensureEntryFromResolved method.

[joaotavora]

Indeed, it was getting lost here because the url, passed to it was already query-trimmed. So I agree with your change here, and yes, I think it means you can revert this change in moduleGraph.ts.

       fileMappedModules.add(mod)
+    } else if (mod.id !== resolvedId) {
+      this.idToModuleMap.set(resolvedId, mod)
+      if (meta) mod.meta = { ...mod.meta, ...meta }
     }
     return mo

But should you? Is not that change conceptually correct in the protocol of ensureEntryFromResolved? Depends on what one takes to be "key" inside resolvedUrl triplet. If you say it's only the URL, you're right to revert it. If you assume its the URL/id combination, you're not.

[aleclarson]

The rationale for removing that block goes like this:

The resolution of a module URL is assumed to be idempotent, which means the resolvedId and meta values are not expected to change between resolveId invocations. This assumption is made clear by the ModuleNode object having only a single id string, rather than an array of them.

[joaotavora]

The resolution of a module URL is assumed to be idempotent, which means the resolvedId and meta values are not expected to change between resolveId invocations.

Makes sense. And you now made it so that that expectation is fulfilled where it wasn't. However, they are still three arguments to the function that will sometimes ignore the latter two if they vary. This means that even with this new somewhat cleaner helper, it could perhaps be even cleaner and take only one argument. Not for this PR though, and maybe just stating what you said in a comment/docstring would probably be more than enough.

[aleclarson]

Why did I change this? By this point, the imported URL is resolved, so using ensureEntryFromUrl is overkill. It pointlessly reruns the resolveId hook chain.

[patak-dev]

We talked about this issue with the team today. Once CI issues and conflicts are resolved, we are good to merge it 👍🏼

[joaotavora]

patak ***@***.***> writes:

We talked about this issue with the team today. Once CI issues and conflicts are resolved, we are good to merge it 👍🏼

To be honest, I don't understand what's causing the CI issues. Before Alec did his changes, the CI passed. Alex then simplified the tests. I'm not saying Alec's changes aren't good: they look fine. But I don't understand what broke. I'm afraid I don't have much time in the coming days to fix it, but I can try if noone beats me to it. João

[aleclarson]

Closing in favor of #7477