Open hugomrdias opened 3 years ago
@rvagg added a feature flag for this but the latest esbuild supports export maps now, would be best to upgrade.
we updated esbuild but we still see these issues. i still need to research more about what is happening here but independent from esbuild if ipjs outputs the browser field like this, it would be useful to do it in a more canonical way.
I'm guessing this browser
field is for bundlers that dont support exports ? im confused about the use case here, because the root browser
takes the bundler to the cjs code and the exports browser
to the esm code.
I'm guessing this browser field is for bundlers that don't support exports?
It's more complicated than that unfortunately. I think js-multiformats is the best example because it has to do things very differently in the browser where implementations used are entirely different (although @ipld/car
does try and do a similar thing).
https://unpkg.com/browse/multiformats@4.6.1/package.json
This works with WebPack, but of course that's just one of many and they all seem to have different opinions. I don't know what the majority opinion is on how to use "browser"
but we're going with an apparent majority opinion here and my initial guess would be that esbuild is in the minority of assuming that it's a mapping of actual files to browser files rather than a mapping of imports/requires to browser files.
We could switch "main"
back on now by compiling with --main
but it'd be nice to avoid that if we can so we don't break other things (TypeScript being the one that broke when we first tried it! although that's apparently fixable). Modifying how "browser"
works though is another matter because it will likely result in breakage in other bundlers (WebPack the known one but I suspect browserify is the same).
There’s a spec for the browser field https://github.com/defunctzombie/package-browser-field-spec
TBH, esbuild is doing some good stuff but the enthusiasm for its performance is a bit premature since it hasn’t actually met the feature set of other build tools. Everywhere I’ve seen it adopted I’ve also seen new problems, many of which have not yet been resolved. It’s promising but still immature.
There’s a spec for the browser field https://github.com/defunctzombie/package-browser-field-spec
I'm the author of esbuild. Unfortunately that spec for the browser field you linked doesn't really specify much, is unmaintained, and comes without any tests, so has been pretty much impossible for me to know what the behavior should be without getting bug reports about cases that don't work. Please feel free to reach out if something isn't working. I'm happy to fix path resolution in esbuild if something is wrong.
I just tried giving this a go and it looks like using the browser field to remap "."
doesn't work with Webpack or Browserify either:
esbuild:
🚫 @ipld/car
Build failed with 1 error:
entry.js:1:8: error: Could not resolve "@ipld/car" (mark it as external to exclude it from the bundle)
✅ @ipld/car/indexed-reader
webpack:
🚫 @ipld/car
ERROR in ./entry.js 1:0-20
Module not found: Error: Can't resolve '@ipld/car' in '/Users/evan/Desktop/foo'
✅ @ipld/car/indexed-reader
browserify:
🚫 @ipld/car
Error: Can't walk dependency graph: Cannot find module '@ipld/car' from '/Users/evan/Desktop/foo/entry.js'
required by /Users/evan/Desktop/foo/entry.js
✅ @ipld/car/indexed-reader
With only the browser
field and not the exports
field, all three cannot resolve @ipld/car
but can resolve @ipld/car/indexed-reader
. So it looks like the way esbuild is handling this is consistent with other implementations of the browser
field in this case?
@defunctzombie @FredKSchott thoughts?
I think Rollup does have some handling for a browser map like you have here (which esinstall/Snowpack/Skypack all take advantage of), but I agree with @evanw that that behavior has always felt undefined to me. imo the sooner you can move over to 100% export maps, the better.
(also: "browser" pointing to CJS instead of ESM? blasphemy! :)
we updated esbuild but we still see these issues
This is a bug in esbuild btw. The bug caused export maps to not work with scoped packages in esbuild. I'll push out the fix for it soon. This isn't surprising since export maps is a brand new feature in esbuild and isn't battle-tested yet.
But even with the fix, the @ipld/car@0.1.2
package hits other issues when it's bundled with esbuild due to the use of built-in node modules in a browser build. I'm guessing this second-order problem is caused by the order of conditions in the exports map:
"exports": {
".": {
"require": "./cjs/car.js",
"import": "./esm/car.js",
"browser": "./esm/car-browser.js"
},
If browser
comes after require
and import
, then the browser
export will never be used since all imports are either a require
or an import
and conditions are checked in the order they appear in package.json
. You'd have to move browser
before require
and import
if you want it to take effect.
Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.
Semantic map key ordering, wonderful idea.. I bet you have fun with that in Go @evanw.
We can get that order switched around here pretty easily I think, I think it would make sense to put "browser"
at the top.
So re the top-level "browser"
, does anyone have any idea how close we are to being able to remove that entirely? Do all the widely-used current-generation bundlers all support "exports"
now?
FYI I already released aegir
with esbuild fix for scoped packages.
So re the top-level
"browser"
, does anyone have any idea how close we are to being able to remove that entirely? Do all the widely-used current-generation bundlers all support"exports"
now?
I suspect Parcel doesn't support "exports"
yet since this is still open: https://github.com/parcel-bundler/parcel/issues/4155.
while debugging and build issue with @ipld/car i noticed that the browser field can't be resolved by esbuild
i think this is trying to mimic how export maps work with abstract entry points to the package but the old
main
andbrowser
fields dont care about that, they just care about "real" files and mapping "real" files to other "real" files. In the snippet above we can see that"."
or"./reader"
etc don't actually exist inside@ipld/car
and so esbuild can't bundle and errors saying cant resolve@ipld/car
.A fix would be to output this instead
Theres other ways to fix this but this seems to be most canonical one.