-
Notifications
You must be signed in to change notification settings - Fork 522
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
feat(esbuild): tool resolution via toolchain (for discussion) #2592
Conversation
This currently works when testing packages/esbuild/test, but not when used via a built package, and I wanted to get an idea of if this is a desired change before proceeding further. The current recommended way to pass the esbuild binary to the rule is to use a select() statement, but that makes a decision based on the target architecture, not the host architecture, and causes builds to fail when a target platform has been set with --platforms that is not one of Mac/Windows/Linux (related: bazel-contrib#2591) By switching to a toolchain, we can specify that each platform's binary requires a given host platform, so if building on a Mac for example, it will invoke the esbuild Mac binary even if a build is targeting another architecture, eg building a native Android/iOS library that needs to bundle esbuild output products. I made some other changes in this PR for discussion - they are not required for toolchain support, but IMHO would be nice to have: - Bundle esbuild_repo.bzl, so users can get easy access to the same esbuild version that the tests are run against. Currently, if you update rules_nodejs after a change like the move to esbuild 0.11, your build will break until you update the local esbuild binaries in your repo to match. The change also makes esbuild behave more like the core nodejs toolchain, which includes some default binary URLs. - Change the script that generates esbuild_repo.bzl to use maybe(), so that users can override the default URLs if they wish. This is not strictly necessary, as the user could define and register their own toolchains instead, but it just makes it a bit easier to override the default values. If there is interest in the toolchain change (and possibly the repo change), I'd appreciate some advice on how to proceed. A toolchains= argument to a rule seems to require an absolute label, so I can't use ":toolchain_type" - it needs to be "//packages/esbuild:toolchain_type" in local development, and something like "@npm//@bazel/esbuild:toolchain_type" when installed via npm. The package.json in examples/esbuild appears to reference npm packages directly, so presumably the examples can't be updated until a new release is made. Is there some trick I can use to replace the npm reference with the files in dist/bin/packages/esbuild/npm_package? I tried updating the package.json file to reference "file:../../dist/bin/packages/esbuild/npm_package", but the build fails, and I'm not sure if that's because it's creating a symlink in the file: case. Is there a recommended way I can test out the changes without having to upload packages to npm? Is this something there'd be interest in including?
Thanks for this! Overall I think this is an approach we'd like (and similar to how it was originally in the initial esbuild PR 😄) There is hurdle to overcome which is caused by how the packages are distributed on NPM. By having the user have the This doesn't particularly affect users with smaller We could probably work around this by having the user have a separate
No, this is updated by the rules_nodejs_integration_test so will take the package from HEAD. You'll want to run |
Ah, thanks for that. I see there is mention of the integration tests in DEVELOPING.md, but I didn't connect the dots. The loss of lazy loading is a shame. Am I correct in assuming that the primary motivation for packages/ is for each subcomponent to be able to depend on their own JS dependencies? esbuild seems to be in a bit of a unique position here in that it's able to function without its own node_modules. Including esbuild/ in internal/ in release.tar.gz would presumably solve the lazy load issue, but not sure how much appetite there is for that, given it's not a core component. |
I wasn't sure how to solve this in a way that would be suitable for inclusion into rules_nodejs, so for now I've just copied packages/esbuild/ into a separate repo and applied the toolchain changes there, so my project can make use of them. |
I think we might have a solution here to avoid the eager loading of We could have an "advanced" usage track here were we do include the files needed to setup the toolchain and rule etc in the npm package, however we instruct users to fetch it via We'd either have to ensure the existing workflow works, or we make this a breaking change in the 4.0.0 release. wdyt? |
If the download URL of npm packages is guaranteed, then that seems like a convenient way of avoiding having to host the package separately. In terms of compatibility, I think it might not be possible to conditionally take a toolchain - even if we continued to support the old binary path if passed in, users would presumably still need to register a dummy toolchain in their workspace as well, or a separate rule definition would be required. So it might make more sense to wait until 4.0.0 - is that planned for any time soon? |
Ah yeah good points. The npm url should be stable, and we can also publish it as a separate release asset to GH, I'd like to do this for the cypress package as it suffers from the same eager loading issues (cc @JesseTatasciore ) We try and do a major release every 6 months, 3.0.0 was released at the end of December, so we are gearing up for a 4.0.0 soon. I can make the 4.x branch and make breaking changes in there. |
Implemented in #2704 |
This currently works when testing packages/esbuild/test, but not when
used via a built package, and I wanted to get an idea of if this is
a desired change before proceeding further.
The current recommended way to pass the esbuild binary to the rule
is to use a select() statement, but that makes a decision based on
the target architecture, not the host architecture, and causes builds
to fail when a target platform has been set with --platforms that is
not one of Mac/Windows/Linux (related: #2591)
By switching to a toolchain, we can specify that each platform's binary
requires a given host platform, so if building on a Mac for example,
it will invoke the esbuild Mac binary even if a build is targeting
another architecture, eg building a native Android/iOS library that
needs to bundle esbuild output products.
I made some other changes in this PR for discussion - they are not
required for toolchain support, but IMHO would be nice to have:
esbuild version that the tests are run against. Currently, if you update
rules_nodejs after a change like the move to esbuild 0.11, your build
will break until you update the local esbuild binaries in your repo
to match. The change also makes esbuild behave more like the core
nodejs toolchain, which includes some default binary URLs.
so that users can override the default URLs if they wish. This is not
strictly necessary, as the user could define and register their own
toolchains instead, but it just makes it a bit easier to override the
default values.
If there is interest in the toolchain change (and possibly the repo change),
I'd appreciate some advice on how to proceed. A toolchains= argument to
a rule seems to require an absolute label, so I can't use ":toolchain_type" -
it needs to be "//packages/esbuild:toolchain_type" in local development, and
something like "@npm//@bazel/esbuild:toolchain_type" when installed via
npm.
The package.json in examples/esbuild appears to reference npm packages
directly, so presumably the examples can't be updated until a new release
is made. Is there some trick I can use to replace the npm reference
with the files in dist/bin/packages/esbuild/npm_package? I tried updating
the package.json file to reference
"file:../../dist/bin/packages/esbuild/npm_package", but the build fails,
and I'm not sure if that's because it's creating a symlink in the file:
case. Is there a recommended way I can test out the changes without having
to upload packages to npm?
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information