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(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories #1369

Merged

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Nov 21, 2019

BREAKING CHANGES:

bazel run @nodejs//:npm is replaced with bazel run @nodejs//:npm_node_repositories and bazel run @nodejs//:yarn is replaced with bazel run @nodejs//:yarn_node_repositories. @nodejs//:yarn and @nodejs//:npm now run yarn & npm in the current working directory instead of on all of the package.json files in node_repositories().

@nodejs//:bin/node & @nodejs//:bin/node.cmd (on Windows) are no longer valid targets. Use @nodejs//:node instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/node.

@nodejs//:bin/yarn & @nodejs//:bin/yarn.cmd (on Windows) are no longer valid targets. Use @nodejs//:yarn instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/yarn.

@nodejs//:bin/npm & @nodejs//:bin/npm.cmd (on Windows) are no longer valid targets. Use @nodejs//:npm instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/npm.


While working on angular/angular#33927 and cleaning up node_repositories.bzl and in particular the aliases in the @nodejs repository, I came across this change which makes the API surface for the @nodejs repository much cleaner and more intuitive.

Users can just call @nodejs//:yarn on all platforms instead of @nodejs//:bin/yarn on osx/linux and @nodejs//:bin/yarn.cmd on Windows. Same difference for npm as well.

The targets @nodejs//:yarn_node_repositories and @nodejs//:npm_node_repositories are also better names for those entry points as they run yarn & npm on all of the package.json files in the node_repositories() repository rule.

Internally, the aliases match up with the actual entry point names so this makes the code much cleaner:

alias(name = "node", actual = "bin/node")
alias(name = "npm", actual = "bin/npm")
alias(name = "yarn", actual = "bin/yarn")
alias(name = "npm_node_repositories", actual = "bin/npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "bin/yarn_node_repositories")

and there is no longer a need to alias the bin/npm and bin/npm.cmd versions of the entry points in @nodejs//:BUILD.bazel as aliases to the aliases is sufficient:

alias(name = "node", actual = "@nodejs_darwin_amd64//:node")
alias(name = "npm", actual = "@nodejs_darwin_amd64//:npm")
alias(name = "yarn", actual = "@nodejs_darwin_amd64//:yarn")
alias(name = "npm_node_repositories", actual = "@nodejs_darwin_amd64//:npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "@nodejs_darwin_amd64//:yarn_node_repositories")

Also,

  • create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets
  • create @nodejs//:node_files, @nodejs//:npm_files & @nodejs//:yarn_files filegroups
  • cleanup & simplify node_repositories.bzl code
  • cleanup & simplify @nodejs//:BUILD.bazel file

@nodejs_darwin_amd64//:BUILD.bazel now looks like this:

# Generated by node_repositories.bzl
package(default_visibility = ["//visibility:public"])
exports_files([
  "run_npm.sh.template",
  "bin/node_repo_args.sh",
  "bin/nodejs/bin/node",
  "bin/nodejs/bin/npm",
  "bin/nodejs/bin/npx",
  "bin/yarnpkg/bin/yarn",
  "bin/node",
  "bin/npm",
  "bin/yarn",
  "bin/npm_node_repositories",
  "bin/yarn_node_repositories",
  ])
alias(name = "node_bin", actual = "bin/nodejs/bin/node")
alias(name = "npm_bin", actual = "bin/nodejs/bin/npm")
alias(name = "npx_bin", actual = "bin/nodejs/bin/npx")
alias(name = "yarn_bin", actual = "bin/yarnpkg/bin/yarn")
alias(name = "node", actual = "bin/node")
alias(name = "npm", actual = "bin/npm")
alias(name = "yarn", actual = "bin/yarn")
alias(name = "npm_node_repositories", actual = "bin/npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "bin/yarn_node_repositories")
filegroup(
  name = "node_files",
  srcs = [":node", ":node_bin"],
)
filegroup(
  name = "yarn_files",
  srcs = glob(["bin/yarnpkg/**"]) + [":node_files"],
)
filegroup(
  name = "npm_files",
  srcs = glob(["bin/nodejs/**"]) + [":node_files"],
)

and

@nodejs//:BUILD.bazel on OSX now looks like this:

# Generated by node_repositories.bzl
package(default_visibility = ["//visibility:public"])
# aliases for exports_files
alias(name = "run_npm.sh.template", actual = "@nodejs_darwin_amd64//:run_npm.sh.template")
alias(name = "bin/node_repo_args.sh", actual = "@nodejs_darwin_amd64//:bin/node_repo_args.sh")
# aliases for other aliases
alias(name = "node_bin", actual = "@nodejs_darwin_amd64//:node_bin")
alias(name = "npm_bin", actual = "@nodejs_darwin_amd64//:npm_bin")
alias(name = "npx_bin", actual = "@nodejs_darwin_amd64//:npx_bin")
alias(name = "yarn_bin", actual = "@nodejs_darwin_amd64//:yarn_bin")
alias(name = "node", actual = "@nodejs_darwin_amd64//:node")
alias(name = "npm", actual = "@nodejs_darwin_amd64//:npm")
alias(name = "yarn", actual = "@nodejs_darwin_amd64//:yarn")
alias(name = "npm_node_repositories", actual = "@nodejs_darwin_amd64//:npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "@nodejs_darwin_amd64//:yarn_node_repositories")
alias(name = "node_files", actual = "@nodejs_darwin_amd64//:node_files")
alias(name = "yarn_files", actual = "@nodejs_darwin_amd64//:yarn_files")
alias(name = "npm_files", actual = "@nodejs_darwin_amd64//:npm_files")

Pre-req for angular/angular#33927 so we can call npm & yarn from within an npm_integration_test like so:

angular_integration_test(
    name = "cli_hello_world_test",
    commands = [
        # "$(location @nodejs//:npm_bin) install",
        # "$(location @nodejs//:npm_bin) run test",
        "$(location @nodejs//:yarn_bin) install",
        "$(location @nodejs//:yarn_bin) test",
    ],
    data = [
        # "@nodejs//:npm_bin",
        # "@nodejs//:npm_files",
        "@nodejs//:yarn_bin",
        "@nodejs//:yarn_files",
    ],
    npm_packages = {
        "//packages/animations:npm_package": "@angular/animations",
        "//packages/common:npm_package": "@angular/common",
        "//packages/compiler:npm_package": "@angular/compiler",
        "//packages/compiler-cli:npm_package": "@angular/compiler-cli",
        "//packages/core:npm_package": "@angular/core",
        "//packages/forms:npm_package": "@angular/forms",
        "//packages/language-service:npm_package": "@angular/language-service",
        "//packages/platform-browser:npm_package": "@angular/platform-browser",
        "//packages/platform-browser-dynamic:npm_package": "@angular/platform-browser-dynamic",
        "//packages/router:npm_package": "@angular/router",
        "//packages/zone.js:npm_package": "zone.js",
        "@npm//typescript": "typescript",
        "@npm//rxjs": "rxjs",
        "@npm//tslib": "tslib",
        "@npm//protractor": "protractor",
        "@npm//@angular/cli": "@angular/cli",
        "@npm//@types/node": "@types/node",
    },
    test_files = ":cli_hello_world_files",
)

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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@gregmagolan gregmagolan force-pushed the fixup_node_repositories branch from 87076e5 to d997b1c Compare November 21, 2019 07:44
@gregmagolan gregmagolan changed the title feat(builtin): create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories Nov 21, 2019
@gregmagolan gregmagolan force-pushed the fixup_node_repositories branch from d997b1c to 6f82338 Compare November 21, 2019 08:12
@alexeagle
Copy link
Collaborator

Generally looks good to me, what are steps before you want review?

"The targets @nodejs//:yarn_node_repositories and @nodejs//:npm_node_repositories are also better names for those entry points as they run yarn & npm on all of the package.json files in the node_repositories() repository rule" - makes me think maybe the word "all" should appear in that rule name?

@gregmagolan gregmagolan marked this pull request as ready for review November 21, 2019 19:42
@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Nov 21, 2019

Its ready for review now. Forgot that it was still in draft.

@nodejs//:yarn_all_node_repositories and @nodejs//:npm_all_node_repositories may be a little verbose. I wonder how many users are even using this feature now that yarn_install() and npm_install() symlink to the user's workspace?

Is the use case of,

node_repositories(["package.json", "foo/package.json", "bar/package.json"])

something that is useful for users?

Perhaps the recommended approach should be:

node_repositories()
yarn_install(name="npm", package_json="package.json")
yarn_install(name="npm2", package_json="foo/package.json")
yarn_install(name="npm3", package_json="bar/package.json")

or a user can also just run yarn or npm multiple times to replace the behavior of @nodejs//:yarn_node_repositories:

bazel run @nodejs//:yarn
bazel run @nodejs//:yarn --cwd foo
bazel run @nodejs//:yarn --cwd bar

@gregmagolan gregmagolan force-pushed the fixup_node_repositories branch from 6f82338 to 5d88741 Compare November 21, 2019 22:42
…yarn/npm]_node_repositories

Also,

* create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets
* create @nodejs//:node_files, @nodejs//:npm_files & @nodejs//:yarn_files filegroups
* cleanup & simplify node_repositories.bzl code
* cleanup & simplify @nodejs//:BUILD.bazel file

BREAKING CHANGES:

bazel run @nodejs//:npm is replaced with bazel run @nodejs//:npm_node_repositories and bazel run @nodejs//:yarn is replaced with bazel run @nodejs//:yarn_node_repositories. @nodejs//:yarn and @nodejs//:npm now run yarn & npm in the current working directory instead of on all of the package.json files in node_repositories().

@nodejs//:bin/node & @nodejs//:bin/node.cmd (on Windows) are no longer valid targets. Use @nodejs//:node instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/node.

@nodejs//:bin/yarn & @nodejs//:bin/yarn.cmd (on Windows) are no longer valid targets. Use @nodejs//:yarn instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/yarn.

@nodejs//:bin/npm & @nodejs//:bin/npm.cmd (on Windows) are no longer valid targets. Use @nodejs//:npm instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/npm.
[dc57136] feat(builtin): create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets
@gregmagolan gregmagolan force-pushed the fixup_node_repositories branch from 5d88741 to cb93b59 Compare November 21, 2019 23:21
@gregmagolan gregmagolan merged commit 01079a3 into bazel-contrib:master Nov 22, 2019
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.

3 participants