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(resolve): refactor package.json handling for deep imports #12461

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Mar 17, 2023

Description

Partial revert of #5665. That PR was not exports friendly and we had to fix it in #10371, but it mixes both "root" package.json and "nearest" package.json, which needs to be handled separately.

This PR refactors in a way that pkg passed to resolveDeepImport and resolvePackageEntry is always the "root" package.json. If further down the chain a resolve logic needs the "nearest" package.json, they can resolve it themselves.

This change shouldn't break resolvePackageEntry (since that's root import only), but changes resolveDeepImport slightly, where:

  • If "root" package.json has no exports field, the browser field (if available) will be derive from the "root" package.json (now), instead of the "nearest" package.json (before)

I think the above is a bug fix though.

More details about `browser`

According to https://github.com/defunctzombie/package-browser-field-spec#alternate-main---basic:

All paths for browser fields are relative to the package.json file location (and usually project root as a result).

It implies that package.json other than "root" should be checked too, but if we respect it, that means we have to check all package.json that's part of the current file path, which is a lot of work even if the browser field was never used.

I think it's fine for now to check the browser field based on "root" package.json only as that's the more common case.

Additional context

No benchmarks 😬 But I noticed this optimization as we're consistently trying to resolve package.json for long deep import paths due to:

const nearestPkgId = [...possiblePkgIds].reverse().find((pkgId) => {
nearestPkg = resolvePackageData(
pkgId,
basedir,
preserveSymlinks,
packageCache,
)!
return nearestPkg
})!


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 PR Title 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.

@bluwy bluwy added p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement labels Mar 17, 2023
@stackblitz
Copy link

stackblitz bot commented Mar 17, 2023

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

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 17, 2023

📝 Ran ecosystem CI: Open

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

@patak-dev
Copy link
Member

Qwik, Nuxt, and Histoire are also currently failing on main (same errors).

But Vitepress against main was successful, so there may be a regression for it 👀

@bluwy
Copy link
Member Author

bluwy commented Mar 17, 2023

I ran the VitePress test locally, and I can get the same error, but it's not always reproducible. When they do fail, I can confirm that the VitePress page has an empty <div id="app"></div> which would fail the test. But I'm not sure how that happens.

I can confirm when running some repros that this PR improves the resolve speed slightly, so it's not slowing down the tests that causes it to timeout. Not really sure what to do here though, we could maybe test again when we start the minor.

I'm guessing it's some sort of race condition, since running that failing test directly via pnpm test-init is less likely to cause the fail.

@patak-dev
Copy link
Member

cc @brc-dd in case you have some time to help us debugging this in VitePress before 4.3

@patak-dev patak-dev added this to the 4.3 milestone Mar 17, 2023
@brc-dd
Copy link
Contributor

brc-dd commented Mar 17, 2023

Yeah, I'll look into this, it's probably just a flaky test IG 👀 Even in our repo, we sometimes have to rerun jobs.

@brc-dd
Copy link
Contributor

brc-dd commented Mar 17, 2023

@patak-dev Hey, can you try rerunning the CI for vitepress and see if it's consistently failing for a specific test? If it's random or some race condition, then feel free to go ahead with the PR, I don't think vite has anything to do with that.

Ah, ok nvm. You guys already triggered it 3 times.

@brc-dd
Copy link
Contributor

brc-dd commented Mar 17, 2023

@patak-dev
Copy link
Member

/ecosystem-ci run vitepress

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 17, 2023

📝 Ran ecosystem CI: Open

suite result
vitepress ❌ failure

@brc-dd
Copy link
Contributor

brc-dd commented Mar 17, 2023

Failed 🫠

@patak-dev
Copy link
Member

/ecosystem-ci run vitepress

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 17, 2023

📝 Ran ecosystem CI: Open

suite result
vitepress ❌ failure

@bluwy
Copy link
Member Author

bluwy commented Mar 17, 2023

Yeah it looks like something in this PR is causing it to happen more frequently, but I can't tell where. Thanks for looking into this though @brc-dd!

@patak-dev
Copy link
Member

/ecosystem-ci run vitepress

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 17, 2023

📝 Ran ecosystem CI: Open

suite result
vitepress ✅ success

@patak-dev
Copy link
Member

/ecosystem-ci run vitepress

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 17, 2023

📝 Ran ecosystem CI: Open

suite result
vitepress ✅ success

@patak-dev patak-dev merged commit 596b661 into main Mar 21, 2023
@patak-dev patak-dev deleted the perf-resolve-pkg-deep branch March 21, 2023 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants