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)

this is the extracted logic from above, replacing the assignments with return

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

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 🤢

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?

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).

I let others chip in, maybe we should try to leave the edge case handling as you proposed.

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.

Ok, sounds good to me. I hope we can remove this special logic altogether at one point.