aspect-build / rules_js

High-performance Bazel rules for running Node.js tools and building JavaScript projects
https://docs.aspect.build/rules/aspect_rules_js
Apache License 2.0
295 stars 100 forks source link

[FR]: Automatically generate binary and library targets #1728

Open jun-sheaf opened 2 months ago

jun-sheaf commented 2 months ago

What is the current behavior?

For library targets, users must use npm_link_all_packages to link libraries before using :node_modules. For binary targets, users need to import the bin object for a library and manually declare it.

Describe the feature

Instead of:

load("@aspect_rules_js//js:defs.bzl", "js_library")
load("@npm//:defs.bzl", "npm_link_all_packages")
load("@npm//:typescript/package_json.bzl", "bin") # This is just an example
load("@aspect_rules_js//js:defs.bzl", "js_run_binary")

npm_link_all_packages()

js_library(
    name = "test",
    deps = [
        ":node_modules/lodash"
    ]
)

bin.typescript_binary(
    name = "typescript",
)

js_run_binary(
    name = "t2",
    tool = ":typescript",
)

let's do

load("@aspect_rules_js//js:defs.bzl", "js_library")
load("@npm//:defs.bzl", "npm_link_all_packages")
load("@npm//:typescript/package_json.bzl", "bin") # This is just an example

js_library(
    name = "test",
    deps = [
        "@npm//:node_modules/lodash" # Or preferably "@npm//node_modules/lodash"
    ]
)

js_run_binary(
    name = "t2",
    tool = "@npm//node_modules/typscript/bin:typescript",
)
gregmagolan commented 2 months ago

The desired API in your example above is nice but its not currently possible since the pnpm-lock.yaml file only says that bins exist and not what the bins are. This means that we can only generate a generic load("@npm//:typescript/package_json.bzl", "bin") in the @npm repository that reads the lock file and not a specific @npm//node_modules/typscript/bin:typescript target as we don't know that typescript is one of the bins when we create the @npm repository

gregmagolan commented 2 months ago

We had an old FR to pnpm to do this but the pnpm team preferred not to include the list of bins in the lock file: https://github.com/pnpm/pnpm/issues/5131

jun-sheaf commented 2 months ago

I see. How about library dependencies? Also, how does rules_js generate typescript_binary if it doesn't know the binaries?

gregmagolan commented 2 months ago

The shape of the node_modules in rules_js is that it is made up of output artifacts that are generated by build targets. This is why npm_link_all_packages() is used as under the hood that stamps out all of the targets for the node_modules tree in that package. Something like @npm//:node_modules/lodash could be generated in theory as an alias to @//:node_modules/lodash tho I think having that alias doesn't add an value.

For the bin.typescript_binary macro, these are generated by the npm_import repository rules which are per npm package. Stamping these out into a BUILD file means that that npm_import has to be eagerly fetched so the repository rule can extract the npm package tarball and read the package.json to determine what the bin entries are.

jun-sheaf commented 2 months ago

Would it be non-sense to allow users to eagerly fetch the npm packages for all of this? This solution wouldn't be very different from npm i. Lazy fetching is indeed nice, but in practice, it's better to just fetch everything which makes the npm dependencies behave "as if they were vendored". Some caching would correct the cost of changing dependencies.

gregmagolan commented 2 months ago

With Bazel I don't think that is the right shape as it is canonical to have fetches of 3p deps be lazy. If you're in a large monorepo, for example, and only working on go, you shouldn't need to fetch any of the deps of the typescript part of the repo.

There is room, however, for a tool to be run outside of Bazel and create a manifest of "bin" files as well as "lifecycle hooks" (which are now needed as of pnpm v9 -- see https://github.com/aspect-build/rules_js/pull/1734) and have that manifest checked into the repository along with the pnpm lock file.

In the pnpm issue linked above re: bin entries, one of the maintainers suggested that approach as preferable to putting bin entry details into the lockfile. That could be a change made upstream in pnpm itself where it creates both the lock file and a manifest file which rules_js then uses. Or it could be a stand-alone tool that could be optional to run for users who want better "bin" aliases and automatic lifecycle hook detection.

jun-sheaf commented 2 months ago

That does sound reasonable! A kind of package-lock-metadata.json. I guess this could be run as a workspace post-install hook.