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

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

Open
jun-sheaf opened this issue May 15, 2024 · 7 comments
Open

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

jun-sheaf opened this issue May 15, 2024 · 7 comments
Labels
blocked Blocked by another issue enhancement New feature or request

Comments

@jun-sheaf
Copy link

jun-sheaf commented May 15, 2024

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",
)
@jun-sheaf jun-sheaf added the enhancement New feature or request label May 15, 2024
@github-actions github-actions bot added the untriaged Requires traige label May 15, 2024
@gregmagolan
Copy link
Member

gregmagolan commented May 15, 2024

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
Copy link
Member

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: pnpm/pnpm#5131

@gregmagolan gregmagolan added blocked Blocked by another issue and removed untriaged Requires traige labels May 15, 2024
@jun-sheaf
Copy link
Author

jun-sheaf commented May 15, 2024

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

@gregmagolan
Copy link
Member

gregmagolan commented May 15, 2024

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
Copy link
Author

jun-sheaf commented May 15, 2024

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
Copy link
Member

gregmagolan commented May 15, 2024

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 #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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another issue enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants