'browser' field in dependent package.json files is not respected by @yarnpkg/esbuild-plugin-pnp

[francisu]

If the browser field is specified in a package.json file of a dependency (not the top level), it's not respected.

This is different from the Vite behavior. (I'm using yarn 3 so I use the PR that addresses this issue: vitejs/vite#1979). Vite out of the box does respect the browser field in dependencies for non yarn pnp environments.

Here is a repo that shows the problem: https://github.com/snapstrat/pino-browser

This issue appears to be related, but is not exactly the same: #1324

One major library where this breaks is the aws-sdk. Resolving this would make esbuild work nicely in this case and help adoption.

Thanks so much for your hard work on esbuild! I'm so happy to be out from under create-react-app/webpack slowness and complexity.

[evanw]

I think the problem is that you are using yarn, and @yarnpkg/esbuild-plugin-pnp doesn't support the browser field: yarnpkg/berry#2987. It seems to work fine if you use npm instead of yarn:

npm install
npm run esbuild

This isn't something esbuild can fix itself because the @yarnpkg/esbuild-plugin-pnp plugin overrides path resolution (by design).

The easy way for you to fix this is to just use npm instead of yarn (or at least to disable Yarn PnP). The hard way would be to add support for the browser field to the @yarnpkg/esbuild-plugin-pnp plugin.

Closing as this is not a problem with esbuild itself.

[francisu]

Closing as this is not a problem with esbuild itself.

Thanks for the fast response!

I know that plugin well and am happy to fix it. I assume it would need to read the package.json file in the each directory in the loader (onLoad) to determine which actual files to get for the browser cases; this would make it have to be invoked for each module load, and I'm hoping that won't hurt performance too badly. I'm guessing esbuild handles the package.json interpretation directly only if it has direct access to the node_modules file system.

[francisu]

BTW - not using yarn is not an option as I depend on some of the unique features of yarn 2+. So I will make this work for the yarn environment.

[francisu]

@evanw Could you consider extending the onLoad() function in the plugin interface to understand about package.json files? So it could call onLoad() for each package.json file (or perhaps add an onLoadPackageJson) function, with a filter as usual. And then the code of esbuild could interpret the package.json files to resolve the browser field and then load the correct modules based on that (since you already do that in the local node_modules case). I think this would benefit not only PnP but other cases where the local file system was not used.

I also want to restate that Vite does work this way successfully with PnP. yarn start works correctly handling nested package.json files with no special configuration.

[evanw]

yarn start works correctly handling nested package.json files with no special configuration

I believe this is because yarn monkey-patches the whole file system API, so everything works as long as it's written in JavaScript and running within the same VM. That's not the case for esbuild obviously, which is written in Go.

Could you consider extending the onLoad() function in the plugin interface to understand about package.json files?

I'm not sure that that's the right interface for this. Package resolution also involves a lot more than just reading package.json files. It also involves searching for node_modules folders, trying various different extensions, and (in esbuild's case) also looking at tsconfig.json files. There may also be additional files that esbuild needs to look at or additional things that esbuild needs to do in the future as npm adds extra features to their package format.

For plugins like this to reuse esbuild's behavior instead of writing their own, what is really needed here is a way to replace esbuild's low-level file system API. It's something that other people have asked for and I think it's reasonable to want it but it's hard to do efficiently both because of the overhead of esbuild being a separate process and because esbuild is multi-threaded while JavaScript is single-threaded so calling out to JavaScript is a speed bump.

One past suggestion for getting file system access to work in the browser efficiently is to pass the entire file system as a map to esbuild up front. That wouldn't help with this situation though obviously. Another solution that could work in this specific case is for esbuild to implement .zip file traversal itself, which yarn could presumably then make use of. It's something I've already considered adding to esbuild.

Anyway I assume the easiest immediate solution would be to add a simple implementation of the browser field to that plugin.

[francisu]

For plugins like this to reuse esbuild's behavior instead of writing their own, what is really needed here is a way to replace esbuild's low-level file system API. It's something that other people have asked for and I think it's reasonable to want it but it's hard to do efficiently both because of the overhead of esbuild being a separate process and because esbuild is multi-threaded while JavaScript is single-threaded so calling out to JavaScript is a speed bump.

I think this one is promising in the longer term. Because of your example, I think that more JS tooling will move to golang, and I think that such low-level file system access plugins can be done in golang rather than JS. If nothing else, for simplicity. And I bet someone in the yarn 2 community would be willing to write a PnP resolver (essentially a zip file reader) in golang which would nicely solve this.

I think the yarn folks are really looking at package management in the right way (having been an early adopter of yarn 2) and supporting what they are doing is a good thing.

@arcanis - WDYT?

[evanw]

I tried adding zip file support to esbuild and it seems pretty easy: #1985. From what I understand, this should solve these types of problems with Yarn (once this is shipped and Yarn's esbuild plugin is updated to let esbuild handle paths in zip files from a given esbuild version onward). I assume this approach would also be as optimal as it can be performance-wise.

That doesn't fix the path resolution part, which would still need to be done by a Yarn plugin. The PnP path resolution code is written to strongly discourage 3rd-party implementations so I assume encoding PnP path resolution logic in esbuild is the wrong thing to do here.

[arcanis]

That doesn't fix the path resolution part, which would still need to be done by a Yarn plugin. The PnP path resolution code is written to strongly discourage 3rd-party implementations so I assume encoding PnP path resolution logic in esbuild is the wrong thing to do here.

If that helps I'm considering switching to a pure JSON format in the next Yarn major (and we'd publish this JSON as a formal spec that third-party resolvers can rely upon). The initial reasoning for keeping it a JS API was the need to be able to extend/correct it as needed (which would have been difficult if we had to coordinates with external projects), but it's been stable for some time now.

[francisu]

That doesn't fix the path resolution part, which would still need to be done by a Yarn plugin. The PnP path resolution code is written to strongly discourage 3rd-party implementations so I assume encoding PnP path resolution logic in esbuild is the wrong thing to do here.

Another thing to consider is having an alternative to onLoad which simply translates the path names to be loaded to the zip file names and can also be called with an array of names to eliminate the performance penalty.

[evanw]

If that helps I'm considering switching to a pure JSON format in the next Yarn major (and we'd publish this JSON as a formal spec that third-party resolvers can rely upon). The initial reasoning for keeping it a JS API was the need to be able to extend/correct it as needed (which would have been difficult if we had to coordinates with external projects), but it's been stable for some time now.

Interesting! That does help. That should open up the possibility of esbuild internalizing this logic.