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

Package entry resolution fails when "browser" is an object #910

Open
rschristian opened this issue Mar 17, 2022 Discussed in #909 · 9 comments
Open

Package entry resolution fails when "browser" is an object #910

rschristian opened this issue Mar 17, 2022 Discussed in #909 · 9 comments

Comments

@rschristian
Copy link
Member

Brought up by #909

When a package's "browser" field is an object, resolve.exports will return the entire object, not a string file path:

When true and "browser" is an object, then legacy() will return the the entire "browser" object.

See resolve.exports#optionsbrowser

This presents an issue in packages like bn.js:

package.json

...
"browser": {
  "buffer": false
},

function resolveLegacyEntry(pkg, path) {
const entry =
_resolveLegacyEntry(pkg, {
browser: true,
fields: ['esmodules', 'modern', 'module', 'jsnext:main', 'browser', 'main']
}) || 'index.js';
return '/' + entry.replace(/^\.?\//, '');
}

entry is { buffer: false }, not an import/file path. Besides the immediate issue of .replace not working, we'll need to fallback to "main" most likely.

@danielweck
Copy link

have you considered https://github.com/lukeed/resolve.exports ?

@rschristian
Copy link
Member Author

Er, already what's used, per my comment above.

Not sure I follow?

@danielweck
Copy link

brain fart, sorry :)

@kvanstee
Copy link

kvanstee commented Apr 9, 2022

Hi rschristian, how can this be resolved? My browser balks at bn.js. I don't fully understand the problem.

@rschristian
Copy link
Member Author

@kvanstee Well ideally I think resolve.exports would return the "main" field when "browser" is formed as it is with bn.js (object, without an entry point), though I don't know what Luke thinks of that or if it's even feasibly correct.

I don't know if we correctly handle the "browser" spec as-is right now (not including this instance, which we definitely don't), so we could just strip it out or fallback to "main"/index.js in the case of it being an object.

@kvanstee
Copy link

@rschristian I notice a previous unresolved issue #784 mentions the same code that you do. Also there is a comment that a commit on npm-refactor2 solves the issue. Will the commit solve this issue as well (if it's not the same)? I have cloned npm-factor2 but can't get the yarn commands to compile the code. Do you have a wmr.cjs that does not break with bn.js?

@rschristian
Copy link
Member Author

rschristian commented Apr 23, 2022

Ah yep, good find. This is indeed a duplicate.

Will the commit solve this issue as well (if it's not the same)?

Will the commit solve the issue? It might? I'd have to go digging through each commit to see which one did it, but that branch replaces the NPM plugin entirely. The change that corrects the issue is likely built upon a whole lot of other changes.

You can likely clone WMR's main branch and snip out 'browser' from the fields list to get a working build. Ah, nope. It relies upon buffer which WMR will not provide. Don't have a good answer for you at the moment, besides that you may want to look at other tools if you really need bn.js

@bakeiro
Copy link

bakeiro commented May 25, 2022

Hi!
I would like to use this library for my projects, but can't really do it with this bug... there is any update on this? :)

@rschristian
Copy link
Member Author

@bakeiro There's no one actively working on WMR these days, no. Unless you'd like to submit a fix, it's unlikely to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants