-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
'browser' field in dependent package.json files is not respected by @yarnpkg/esbuild-plugin-pnp
#1983
Comments
I think the problem is that you are using yarn, and
This isn't something esbuild can fix itself because the 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 Closing as this is not a problem with esbuild itself. |
@yarnpkg/esbuild-plugin-pnp
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. |
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. |
@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 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. |
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.
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 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 Anyway I assume the easiest immediate solution would be to add a simple implementation of the |
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? |
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. |
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. |
Another thing to consider is having an alternative to |
Interesting! That does help. That should open up the possibility of esbuild internalizing this logic. |
If the
browser
field is specified in apackage.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.
The text was updated successfully, but these errors were encountered: