fix(resolve): respect order of browser in mainFields when resolving #15137
Conversation
| // fallback to mainFields if still not resolved | ||
| if (!entryPoint) { | ||
| for (const field of options.mainFields) { | ||
| if (field === 'browser') continue // already checked above | ||
| if (field === 'browser' && targetWeb) { |
There was a problem hiding this comment.
This is the same conditional as the previous line 986,
- entryPoint not set (otherwise loop break would have happened)
- targetWeb is true
- browser is included in mainFields (current value of field)
| @@ -1257,6 +1220,53 @@ function tryResolveBrowserMapping( | |||
| } | |||
| } | |||
|
|
|||
| function tryResolveBrowserEntry( | |||
There was a problem hiding this comment.
this is the extracted logic from above, replacing the assignments with return
| // fallback to mainFields if still not resolved | ||
| if (!entryPoint) { | ||
| for (const field of options.mainFields) { | ||
| if (field === 'browser') continue // already checked above | ||
| if (field === 'browser' && targetWeb) { |
There was a problem hiding this comment.
I think the intention was to check browser and module without respecting their order. After this PR, if there is a custom mainFields: ['module', 'browser'] with them inverted, before we were still resolving, and now we will directly use the module field. Maybe we should kick the edge case handling if we hit any of them. It should still fix your issue.
Something like having this out of the loop
const mainFieldsIncludesBrowser = options.mainFields.includes('browser')and then
| if (field === 'browser' && targetWeb) { | |
| if (targetWeb && (field === 'browser' || (field === 'module' && mainFieldsIncludesBrowser) ) { |
There was a problem hiding this comment.
this would be equally weird though, if the resolve order is "module" first, "browser" later, it should not prefer the browser value. Thats a configuration choice that should be respected. Tbh this whole thing probably belongs in a "weird-legacy-resolve" pre plugin that users can install as needed.
The logic inside only kicks in if browser has an entry. what if browser has no entry but module has one? then respecting the new order would work but keeping the old behavior would break. There is no univerally right approach with this. Ultimately i hope only very few libraries have pkg.browser and pkg.module and those two disagree 🤢
There was a problem hiding this comment.
If browser is before module, that should also be respected, even if it is the default. I agree with you, but we missed the opportunity to remove this edge case handling in vite 5, maybe we could in the next major (or try in a minor with a flag to add it back?). For getting this in a patch, probably better to keep it working as close as it was before.
About the second paragraph. If there is no browser entry, the previous logic wont kick in, and the new either, no?
There was a problem hiding this comment.
This would only happen if package.json looked like this
"browser": {"foo":"dist/foo.js"},
"module":"dist/bar.js"
browser would not have an entry, but module does. If we went with the custom logic for module too, it would no longer resolve the valid entry from module.
I really hope there are very few packages that do this kind of stuff and even less setups that put module before browser. Ultimately i'm fine with a patch that allows us to add the "svelte" condition first without this interfering.
I do believe the more compatible thing to do is to follow the order in mainFields mostly and only massage the value of browser in place (and maybe module, but then module probably needs its own logic that works a bit differently than the one for browser).
There was a problem hiding this comment.
I let others chip in, maybe we should try to leave the edge case handling as you proposed.
There was a problem hiding this comment.
I didn't see the special browser handling this way initially, but I do see the point 🤔 I've gone back and forth while writing this comment. While I think patak's idea is great too, it does complicate the implementation.
So I'm leaning towards handling only if browser if found like this PR for now. The new browser string is only supported in Vite 5, and I doubt someone flips it today for any reason. If the reason is to have module take precedence over browser, that wouldn't work anyways before this PR.
There was a problem hiding this comment.
Ok, sounds good to me. I hope we can remove this special logic altogether at one point.
…d entirely when targetWeb is false
|
just pushed a fix for a blunder I made which would have resolved the browser field even if targetWeb was false. The tests did not catch this, so to answer my own question above: Yes we need tests covering the resolve of browser in different scenarios. |
…ly taken into account when targetWeb is true
|
Haven't checked deeply yet. But these are the related discussions I'm aware of. |
…ith custom browser handling
Yeah I don't think we can remove the special heuristic yet. The packages aren't inherently incorrectly packaged I think, just that it wasn't clear in the past what |
Description
fixes #15134
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).