Add npm_package_bin rule #1139
Add npm_package_bin rule #1139
Conversation
alexeagle
force-pushed
the
npm_bin
branch
4 times, most recently
from
aae863c to
ee158b9
Compare
| "test.styl", | ||
| "--out", | ||
| # Output paths must use $(location) since Bazel puts them in a platform-dependent output directory | ||
| "$(location test.css)", |
There was a problem hiding this comment.
As a user, can I use a relative path here such as ../test.css? Not that it makes sense in this example but in general is that possible?
There was a problem hiding this comment.
You linked the docs which answers the question :) https://docs.bazel.build/versions/master/be/general.html#genrule.srcs
There was a problem hiding this comment.
yeah, this will be tough for users I think, because if you just write "test.css" it will write to the input folder. if you're lucky you get an error about that folder being readonly in the sandbox, more likely it writes it and you get an error that bazel says the declared output wasn't produced.
But this is standard genrule behavior so not strictly our job to improve that?
| fail("You must supply either the tool or package attribute") | ||
| if not package_bin: | ||
| package_bin = package | ||
| tool = "@npm//%s/bin:%s" % (package, package_bin) |
There was a problem hiding this comment.
Seems like this could allow a bit more flexibility by allow user to override the @npm workspace without specifying the whole tool? But maybe having the tool override is sufficient
There was a problem hiding this comment.
hmm, we could easily have another attribute for the package workspace. but that does make the API surface of this rule bigger just to preserve some syntax sugar, which seems probably wrong to me.
|
|
||
| # Generated helper macro to call ${name} | ||
| def ${name}(**kwargs): | ||
| npm_package_bin(package = "${pkg._dir}", package_bin = "${name}", **kwargs) |
There was a problem hiding this comment.
I think set the specific tool here instead of package and package_bin as the @npm workspace may not be @npm for this npm_install/yarn_install
| @@ -33,6 +33,8 @@ npm_package( | |||
| srcs = [ | |||
| "@npm_bazel_less//:package_contents", | |||
| ], | |||
| # less package is deprecated | |||
| tags = ["do-not-publish"], | |||
There was a problem hiding this comment.
Update release script to key off of this tag?
There was a problem hiding this comment.
it already does, that's how we avoided publishing @bazel/rollup and @bazel/terser to this point.
|
This is really nice and clean. Nice work! Naming makes sense to me. I think another PR we can have inform npm_install & yarn_install via the @bazel/typescript package.json how to add a ts_library macro to index.bzl with the appropriate @npm_wrksp override as discussed here: #1149 (comment). Ultimately it would be really nice if users can load all npm delivered bazel rules from |
This is a genrule-like rule that runs any npm bin We generate one of these in the @npm workspace illustrate its usage by replacing stylus and less rules Those rules are now deprecated, will be deprecated on npm, and their sources deleted soon. Design doc: https://hackmd.angular.io/f8OBi9KMTNKButSWsh1gCA?view
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