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

feat(esbuild): tool resolution via toolchain (for discussion) #2592

Closed
wants to merge 1 commit into from

Conversation

dae
Copy link
Contributor

@dae dae commented Apr 7, 2021

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:

  • 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?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

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?
@google-cla google-cla bot added the cla: yes label Apr 7, 2021
@mattem
Copy link
Collaborator

mattem commented Apr 7, 2021

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 load statement from the @npm//@bazel/esbuild package in their WORKSPACE file, this currently causes an eager fetch of everything in the @npm repo, whether it's needed by the build or not, as it's needed in order to evaluate the WORKSPACE file.

This doesn't particularly affect users with smaller package.json files or JS only repos, as bazel will be fetching those anyway, but it causes a lot of overhead if for example the user is running bazel build //some/java/thing and is now waiting to download node_modules that are never used.

We could probably work around this by having the user have a separate package.json file for the @bazel/esbuild package, but that would require some clear documentation and reasoning.
Another option is to not distribute the esbuild package on NPM, and have it as a regular http_archive, but we don't have an existing mechanism for that in the repo.

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

No, this is updated by the rules_nodejs_integration_test so will take the package from HEAD. You'll want to run bazel test //examples:examples_esbuild

@dae
Copy link
Contributor Author

dae commented Apr 7, 2021

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.

@dae
Copy link
Contributor Author

dae commented Apr 15, 2021

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.

@mattem
Copy link
Collaborator

mattem commented May 9, 2021

I think we might have a solution here to avoid the eager loading of @npm dependencies in the workspace.

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 http_archive (either from the npm registry url or we start publishing sperate .tgz files to github), rather than having it in the package.json for npm to handle. This way the regular bazel semantics take over and we can include the extra files needed.

We'd either have to ensure the existing workflow works, or we make this a breaking change in the 4.0.0 release.

wdyt?

@dae
Copy link
Contributor Author

dae commented May 10, 2021

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?

@mattem
Copy link
Collaborator

mattem commented May 10, 2021

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.

@mattem
Copy link
Collaborator

mattem commented May 28, 2021

Implemented in #2704

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

Successfully merging this pull request may close these issues.

2 participants