Skip to content

refactor!: remove resolvePackageEntry and resolvePackageData APIs#14584

Merged
bluwy merged 2 commits intomainfrom
remove-package-internal
Oct 13, 2023
Merged

refactor!: remove resolvePackageEntry and resolvePackageData APIs#14584
bluwy merged 2 commits intomainfrom
remove-package-internal

Conversation

@bluwy
Copy link
Copy Markdown
Member

@bluwy bluwy commented Oct 11, 2023 •
edited
Loading

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

sapphi-red commented Oct 11, 2023

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

I guess it's #12957 (comment).

sapphi-red
sapphi-red reviewed Oct 11, 2023
View reviewed changes
patak-cat
patak-cat previously approved these changes Oct 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@patak-cat patak-cat 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.

@bluwy @sapphi-red
docs: update package -> pkg
0797859 
Co-authored-by: 翠 / green <[email protected]>
@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Oct 11, 2023 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor

aleclarson commented Oct 11, 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?

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

@bluwy
Copy link
Copy Markdown
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

This was referenced Oct 13, 2023
Merged
Merged
@bluwy
Copy link
Copy Markdown
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
@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

Reviewers

@patak-cat patak-cat patak-cat approved these changes

@sapphi-red sapphi-red sapphi-red approved these changes

+1 more reviewer

@aleclarson aleclarson aleclarson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bluwy @sapphi-red @aleclarson @patak-cat