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

refactor!: remove resolvePackageEntry and resolvePackageData APIs #14584

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 11, 2023

Description

They are preventing caching optimizations that we can do. IIRC the setResolvedCache API isn't correct too and is causing bugs. (have to find the issue #12957 (comment))

Additional context

After a lot of yak shaving, I finally got to what I wanted to do!


Admittedly I didn't plan this ahead since it would be better with a deprecation phase, but I think it's fine as these APIs are rather internal.

GitHub search for resolvePackageEntry and resolvePackageData usage

Unfortunately resolvePackageEntry affects some Vite plugins, and resolvePackageData affects two big projects: qwik and slidev. But their usage are simple enough that the migration guide should work.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@sapphi-red
Copy link
Member

IIRC the setResolvedCache API isn't correct too and is causing bugs. (have to find the issue)

I guess it's #12957 (comment).

docs/guide/migration.md Outdated Show resolved Hide resolved
patak-dev
patak-dev previously approved these changes Oct 11, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Sounds good to me! You're really pushing hard to clean up things in Vite 😄
It would be good to see what Anthony thinks given that it affects sli.dev though. And I wonder if Vite should export similar utilities in the future (once optimizations are done and you are happy with the new API). It seems useful for plugins to get the exact same handling of package.json as Vite and also take advantage of the cache.

Co-authored-by: 翠 / green <[email protected]>
@bluwy
Copy link
Member Author

bluwy commented Oct 11, 2023

Yeah once this is unblocked we should be able to refine the API to be public again for sure 👍 (especially the package.json handling). I have also thought about creating new @vitejs/* packages to really generalize it, but maybe that's for another day.

@bluwy
Copy link
Member Author

bluwy commented Oct 11, 2023

I think we can hold off merging for now and get qwik and slidev migrated first.

@aleclarson
Copy link
Member

Out of curiosity, is there a detailed explanation of the optimizations blocked by this API, the bugs caused by it, and how removing it will fix them?

From a quick look, I have no packages depending on the API.

@bluwy
Copy link
Member Author

bluwy commented Oct 12, 2023

Out of curiosity, is there a detailed explanation of the optimizations blocked by this API, the bugs caused by it, and how removing it will fix them?

  1. We need to modify the PackageCache to only cache the package.json object, not PackageData.
  2. We need two package cache with different value types. Right now we're mixing both for compat. The keys are prefixed like so (one and two) to workaround it.
  3. 预览模式中element-ui el-table jsx写法不显示 #12957 (comment) highlights the bug that we can't rely on a single resolve cache for each package. We might need to remove PackageData completely, or cache it in a different way.
  4. And IIRC it also blocks us being able to cache for CSS import resolvers

@bluwy
Copy link
Member Author

bluwy commented Oct 13, 2023

I've sent the PRs upstream to remove them. Will go ahead and merge this.

@bluwy bluwy merged commit 339f300 into main Oct 13, 2023
10 checks passed
@bluwy bluwy deleted the remove-package-internal branch October 13, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants