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): make Starlark build settings usable as defines #3122

Merged
merged 1 commit into from
Dec 16, 2021

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Dec 6, 2021

Adds a define_settings attribute to the esbuild rules that allows using the values of Starlark build settings to globally replace
specified identifiers.

The value of the setting is automatically converted to a JS literal.

Note: This currently uses json.encode, which is only available from Bazel 5 on. If this addition is generally viewed favorably, I would look into "polyfilling" this functionality in Starlark.

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: #3113

What is the new behavior?

The existing esbuild rule now has the define_settings attribute described in the issue.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Dec 6, 2021
@fmeum fmeum force-pushed the esbuild-define-settings branch 5 times, most recently from cad446f to 1b5c582 Compare December 6, 2021 20:39
@fmeum
Copy link
Contributor Author

fmeum commented Dec 6, 2021

The buildkite build fails for the examples since they explicitly reference rules_nodejs 4.4.6, which of course does not yet include the required change to the esbuild_repositories macro introduced by this PR. Is this check expected to fail for some types of changes?

@alexeagle
Copy link
Collaborator

There's a bit of magic, the integration test runner replaces those versions with the local_repository(path=../..) like you'd expect. Your code changes should be showing up inside the example, so probably something is actually wrong.

@fmeum
Copy link
Contributor Author

fmeum commented Dec 7, 2021

There's a bit of magic, the integration test runner replaces those versions with the local_repository(path=../..) like you'd expect. Your code changes should be showing up inside the example, so probably something is actually wrong.

The version of skylib fetched in esbuild_repositories was too old. I updated all reference to bazel_skylib to 1.1.1.

@fmeum
Copy link
Contributor Author

fmeum commented Dec 7, 2021

Is there a way to get the testlogs of the inner Bazel execution in an e2e test? The Windows test fails at //:test_acorn, but without the logs its hard to tell why.

Adds a `define_settings` attribute to the `esbuild` rules that allows
using the values of Starlark build settings to globally replace
specified identifiers.

The value of the setting is automatically converted to a JS literal.
Note: This currently uses json.encode, which is only available from
Bazel 5 on. If this addition is generally viewed favorably, I would
look into "polyfilling" this functionality in Starlark.
@fmeum
Copy link
Contributor Author

fmeum commented Dec 7, 2021

Is there a way to get the testlogs of the inner Bazel execution in an e2e test? The Windows test fails at //:test_acorn, but without the logs its hard to tell why.

I noticed that the e2e/core WORKSPACE referred to a bazel_skylib patch, which I apparently dropped with the update. I now changed my commit to only update the bazel_skylib dependency in esbuild_repositories and the tests pass. This could potentially clash with rules_nodejs_dependencies now, but maybe that macro has been deprecated? I don't see it mentioned in the docs anymore.

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alexeagle alexeagle merged commit e47f339 into bazelbuild:stable Dec 16, 2021
@fmeum fmeum deleted the esbuild-define-settings branch December 16, 2021 01:55
alexeagle pushed a commit that referenced this pull request Dec 18, 2021
Adds a `define_settings` attribute to the `esbuild` rules that allows
using the values of Starlark build settings to globally replace
specified identifiers.

The value of the setting is automatically converted to a JS literal.
Note: This currently uses json.encode, which is only available from
Bazel 5 on. If this addition is generally viewed favorably, I would
look into "polyfilling" this functionality in Starlark.
@renovate renovate bot mentioned this pull request Nov 14, 2023
1 task
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.

None yet

2 participants