Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: shorcircuit resolve in ensure entry from url #12655

Merged
merged 5 commits into from
Mar 30, 2023

Conversation

patak-dev
Copy link
Member

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
Copy link

stackblitz bot commented Mar 29, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy
Copy link
Member

bluwy commented Mar 29, 2023

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

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

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

bluwy commented Mar 30, 2023

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 patak-dev marked this pull request as draft March 30, 2023 07:25
@patak-dev
Copy link
Member Author

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 patak-dev marked this pull request as ready for review March 30, 2023 14:05
@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 30, 2023

📝 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 patak-dev requested a review from bluwy March 30, 2023 18:14
@patak-dev
Copy link
Member Author

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
Copy link
Member Author

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.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give this a shot 👍

@patak-dev patak-dev enabled auto-merge (squash) March 30, 2023 18:21
@patak-dev patak-dev merged commit 82137d6 into main Mar 30, 2023
@patak-dev patak-dev deleted the perf/shortcircuit-resolve-in-ensure-entry-from-url branch March 30, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants