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: update node toolchain to provide File objects #3736

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Apr 2, 2024

To-date the node toolchain has provided paths (strings) to node and npm. This complicates downstream usage as they need to convert these paths depending on they are used (between execpath and rootpath for example). This change configures the toolchain to also provide File objects for node and the npm entry point script so downsteam uses can choose to get either the .path or .short_path as needed.

Deprecates target_tool and target_tool_path parameters. Maintains backward compat by setting both target_tool_path and tool_files in the provider.

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?

  • Feature (please, look at the "Scope of the project" section in the README.md file)

Does this PR introduce a breaking change?

  • Yes
  • No

@gregmagolan gregmagolan changed the title refactor: update node toolchain to provider File objects instead of paths feat: update node toolchain to provide File objects Apr 2, 2024
@gregmagolan gregmagolan force-pushed the provides_files branch 6 times, most recently from b5deb51 to af4c47b Compare April 2, 2024 16:16
nodejs/toolchain.bzl Outdated Show resolved Hide resolved
nodejs/toolchain.bzl Outdated Show resolved Hide resolved
@gregmagolan gregmagolan force-pushed the provides_files branch 2 times, most recently from 4a920ff to 5d7875c Compare April 2, 2024 16:36
@jbedard
Copy link
Collaborator

jbedard commented Apr 2, 2024

LGTM, but if we are avoiding breaking changes how will other rulesets take advantage of this? Especially if we still want to support the *_path options everywhere won't we still have the same path vs short_path problem?

@gregmagolan
Copy link
Collaborator Author

LGTM, but if we are avoiding breaking changes how will other rulesets take advantage of this? Especially if we still want to support the *_path options everywhere won't we still have the same path vs short_path problem?

rules_js will need to be both backward and forward compatible; preferring the Files in the provider over the paths if they are there. Can't have your cake and eat it too without breaking changes.

@gregmagolan gregmagolan requested a review from alexeagle April 2, 2024 16:54
@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Apr 2, 2024

In the future when we drop WORKSPACE support here and in rules_js then all of this is unwound and removed.

I'm hoping to be bzlmod only in all of our rule sets after Bazel 8 is released.

nodejs/toolchain.bzl Outdated Show resolved Hide resolved
nodejs/toolchain.bzl Outdated Show resolved Hide resolved
@alexeagle alexeagle merged commit 176e900 into main Apr 3, 2024
4 checks passed
@alexeagle alexeagle deleted the provides_files branch April 3, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants