perf(resolve): improve package.json resolve speed#12441
Conversation
|
|
| * 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.
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! |
patak-cat
left a comment
There was a problem hiding this comment.
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 theresolvepackage to resolve deppackage.jsonfiles, 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
resolvepackage 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
resolvedep 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).