Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: log warning if svelte field causes difference in resolve #510
feat: log warning if svelte field causes difference in resolve #510
Changes from 9 commits
ebe18d4
81bfa52
f8c856b
4a6f5f0
8335144
4468262
eb933fe
3f86ab0
63fe891
e24d839
c608797
8a46847
c66e2f5
d1e7e2d
4771880
13abab4
e1b1e6d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
per @dummdidumm's point, I'm not sure we need to actually deprecate the
svelte
field. we just need to stop resolving it beforeexports
and resolve it after instead. that would make the warning trigger less frequently which would be nicer for usersThere 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.
actually, I just looked at the code and I think it works assuming the svelte field will be left in place and only changes the order. if we really did want to get rid of it entirely we'd probably have to change the code, but I'd just suggest changing the title here and note below saying it will be removed and instead say the resolve order will change
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 don't think we can recover from a resolve error unless we always call
this.resolve
in a try/catch not something i want to do long term.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.
also having 2 accepted ways to do this doesn't feel great. it makes writing new bundler plugins even more challanging. The only point i see in this whole excercise is that at the end of it the svelte field is no longer used at all.
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.
There's already two ways to resolve JS files with
main
andexports
, so it's just the analogous thing. As long as we preferexports
we can rip out all the code invite-plugin-svelte
and just defer to ViteThere 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.
@bluwy iirc you changed the behavior of exports + mainfields lately. Is the above still true? If yes we should be able to remove the custom code for resolveViaPackageJsonSvelte even today.
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.
can we change the title from
deprecated "svelte" field
to something more neutral likeprefer "svelte" export condition over "svelte" field
?I agree with what you said in that comment and so the word "deprecated" seems too strong to me
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.
If
exports
is define, Vite should ignoremainFields
(exceptbrowser
) completely now. Vite also doesn't have a restriction on what the extensions formainFields
could be, so exporting a Svelte component is fine too.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.
@dominikg any thoughts about the heading title here? I think it might be the only outstanding item for us to resolve
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 have changed the wording to use conflicting resolve results instead of deprecation.