perf: shorcircuit resolve in ensure entry from url

[patak-dev]

Description

Implement shortcuts based on this idea by @sapphi-red

Extracted from #12635

Using a private function to avoid extending the API yet. Maybe we should expose it if it could help Vitest or plugins in the future.

Perf should increase to #12635 (comment), if this PR and #12635 are merged.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[bluwy]

It seems that the module graph's resolver, and the import analysis's resolver differs slightly though:

vite/packages/vite/src/node/server/index.ts

Lines 374 to 376 in 9697e64

	const moduleGraph: ModuleGraph = new ModuleGraph((url, ssr) =>
	container.resolveId(url, undefined, { ssr }),
	)

vite/packages/vite/src/node/plugins/importAnalysis.ts

Line 320 in 9697e64

const resolved = await this.resolve(url, importerFile)

I'm not sure if it's safe to use the pre-resolved value 🤔 Might need to see tests when GH is back.

[patak-dev]

You are right. The result of the two calls won't be exactly the same. The meta could be different in the two resolve calls. The id should be the same though:

In importAnalysis we do this.resolve(url, importerFile) but that call internally calls resolveId with the ssr flag set correctly.

We need importerFile because url at this point could be extensionless or relative, or it could also be a virtual module, an alias, etc. Once we call this, we are finding the url we will use to overwrite the original here.

This new url we pass to ensureEntryFromUrl is derived from the id. It is almost the resolved id with some modifications to make it browser friendly. It should have the proper extension already, be an absolute path, etc. So when we call resolve on it again, it doesn't need the importer and it would resolve as /@fs or absolute path quickly.

About the meta, it was added by #5465. And it is set here. So this PR is a breaking change if someone is returning for the same resolved id a different meta. I would expect that no matter what url you feed to the plugins, if they resolve to the same id, then the other properties (like external, moduleSideEffects, meta, etc) should all be the same. If not, how you decide if an id is external or not if two unresolved ids diverge?

[patak-dev]

Ok, seems there are known issues with the way meta is handled. There is a PR to fix them #7477, but it is hard to merge as it modifies several things (and it is outdated now). I don't think meta works correctly right now, but maybe we should hold on this PR until things are cleaned up.

[bluwy]

Thanks for the writeup! That makes sense to me, if the assumption is that the resolved id should return the same meta, then we could give this a shot.

[patak-dev]

Making this a draft so we avoid merging it yet. But I actually think that using the meta from the resolveId call from importAnalysis is closer to the call from build so this may help some apps (to avoid bugs, meta should be the same though as we discussed)

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	❌ failure
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt	❌ failure
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	❌ failure
vite-plugin-react	✅ success
vite-plugin-react-pages	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	❌ failure
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

[patak-dev]

I think we can move forward with this one. Even if the meta could be different, using the one from importAnalysis is closest to build time. Plugins should resolve the same meta for the same id though, we can't control how the module would be registered in the module graph first.

[patak-dev]

The failures in vite-ecosystem-ci are the same as we see in main.

I think our tests may be a bit flakier after merging this and the unresolvedUrlToModule cache PR, but IMO that is because we will be uncovering issues with the tests.

[bluwy]

Let's give this a shot 👍