-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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(resolve): improve package.json resolve speed #12441
Conversation
Run & review this pull request in StackBlitz Codeflow. |
* if importer was not resolved by vite's resolver previously | ||
* (when esbuild resolved it) | ||
* resolve importer's pkg and add to idToPkgMap | ||
* Load closest `package.json` to `importer` | ||
*/ | ||
function resolvePkg(importer: string, options: InternalResolveOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to refactor this function, since it's incorrectly using resolvePackageData
and looks unnecessarily complex 🤔 What happens is that this function gets the nearest package.json
, and loads the PackageData
. And the new changes does the same too.
In the old changes, it manually walks /User/foo/project
, create a possiblePkgIds
of ['/User/foo/project', '/User/foo', '/User']
, and then try to resolve the package.json
in those directories. So resolvePackageData
would receive absolute paths instead of bare imports which doesn't work with this refactor.
Nice work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredible work 🔥
Changes look good to me, thanks for breaking them down into simpler commits.
It feels a bit big to include in a patch. But it is tempting to do so. If we don't, we could start the 4.3 beta soon after we give some time to 4.2 to stabilize.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
The same projects are failing compared to main (same error in Nuxt). I think we should merge and release tomorrow as 4.2.1 so we can build on top of this PR without waiting for 4.3. |
I like the simple approach this has 👍 |
Looks amazing🔥. I think we should implement this performance improving! |
A little late to the party, but LGTM. (Also 👍 to get it out in the field sooner rather than lather.) Neat PR 🔥. |
Commenting at the request of @bluwy - LGTM and shouldn't be a blocker in any way for Deno plugin. |
Description
Should fix #8850, my benchmarks were based on on the issue's repro.
This PR bring 1.5-4x resolve speed for resolving modules that heavily rely on resolving
package.json
. We only use theresolve
package to resolve deppackage.json
files, I've implemented a simpler logic for it specifically, extracted from vitefu, which has been used by many frameworks so far.This simpler logic is more straight-forward and prevents the use of errors + try/catch. It also supports yarn pnp. (fun fact: yarn pnp patches the
resolve
package by default)Additional context
It would be easier to review by each commit.
Before merging this, it might be a good idea to run ecosystem-ci.
Here are the cpuprofiles: https://gist.github.com/bluwy/19c6c3ec6fc3479ffa5e679b4c61344e
The
resolve
dep is not removed yet. There's one more place that needs to be fixed before doing so, the work is tracked at #11410. This shouldn't affect anything though.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).