feat(builtin): more idiomatic pkg_npm

- require that we know the package_name (defaults to name)
- require that we know the package.json file (or generate one if not)
- give a buildozer command to sync the package.json name field into the BUILd file

index.bzl

@@ -30,7 +30,7 @@ load(
 load("//internal/node:node_repositories.bzl", _node_repositories = "node_repositories")
 load("//internal/node:npm_package_bin.bzl", _npm_bin = "npm_package_bin")
 load("//internal/npm_install:npm_install.bzl", _npm_install = "npm_install", _yarn_install = "yarn_install")
-load("//internal/pkg_npm:pkg_npm.bzl", _pkg_npm = "pkg_npm")
+load("//internal/pkg_npm:pkg_npm.bzl", _pkg_npm = "pkg_npm_macro")
 load("//internal/pkg_web:pkg_web.bzl", _pkg_web = "pkg_web")
 
 check_bazel_version = _check_bazel_version

internal/pkg_npm/BUILD.bazel

@@ -12,6 +12,12 @@ bzl_library(
 # Exported to be consumed for generating stardoc.
 exports_files(["pkg_npm.bzl"])
 
+nodejs_binary(
+    name = "validator",
+    entry_point = "pkg_npm_validator.js",
+    visibility = ["//visibility:public"],
+)
+
 nodejs_binary(
     name = "packager",
     data = ["//third_party/github.com/gjtorikian/isBinaryFile"],

internal/pkg_npm/pkg_npm.bzl

@@ -6,7 +6,7 @@ If all users of your library code use Bazel, they should just add your library
 to the `deps` of one of their targets.
 """
 
-load("//:providers.bzl", "DeclarationInfo", "JSNamedModuleInfo", "LinkablePackageInfo", "NodeContextInfo")
+load("//:providers.bzl", "DeclarationInfo", "JSNamedModuleInfo", "LinkablePackageInfo", "NodeContextInfo", "run_node")
 
 _DOC = """The pkg_npm rule creates a directory containing a publishable npm artifact.
 
@@ -75,7 +75,10 @@ You can pass arguments to npm by escaping them from Bazel using a double-hyphen,
 # Used in angular/angular /packages/bazel/src/ng_package/ng_package.bzl
 PKG_NPM_ATTRS = {
     "package_name": attr.string(
-        doc = """Optional package_name that this npm package may be imported as.""",
+        doc = """Value of the "name" key in package.json.
+        Determines the name of the package if it is published to a registry.
+        Also causes the package to be npm-link'ed into Bazel rules that depend on it.""",
+        mandatory = True,
     ),
     "srcs": attr.label_list(
         doc = """Files inside this directory which are simply copied into the package.""",
@@ -90,6 +93,7 @@ PKG_NPM_ATTRS = {
         providers = [NodeContextInfo],
         doc = "Internal use only",
     ),
+    "package_json": attr.label(allow_single_file = [".json"]),
     "replace_with_version": attr.string(
         doc = """If set this value is replaced with the version stamp data.
         See the section on stamping in the README.""",
@@ -128,6 +132,8 @@ PKG_NPM_OUTPUTS = {
     "publish": "%{name}.publish",
 }
 
+_ValidationMarkerInfo = provider()
+
 # Takes a depset of files and returns a corresponding list of file paths without any files
 # that aren't part of the specified package path. Also include files from external repositories
 # that explicitly specified in the vendor_external list.
@@ -147,8 +153,7 @@ def _filter_out_external_files(ctx, files, package_path):
 def create_package(ctx, deps_files, nested_packages):
     """Creates an action that produces the npm package.
 
-    It copies srcs and deps into the artifact and produces the .pack and .publish
-    scripts.
+    It copies srcs and deps into the artifact.
 
     Args:
       ctx: the skylark rule context
@@ -164,6 +169,13 @@ def create_package(ctx, deps_files, nested_packages):
     stamp = ctx.attr.node_context_data[NodeContextInfo].stamp
 
     all_files = deps_files + ctx.files.srcs
+    all_srcs = ctx.files.srcs[:]
+    if ctx.attr.package_json:
+        all_srcs.append(ctx.file.package_json)
+    else:
+        generated_pkg_json = ctx.actions.declare_file("package.json")
+        ctx.actions.write(generated_pkg_json, struct(name = ctx.attr.package_name).to_json())
+        deps_files.append(generated_pkg_json)
 
     if not stamp and len(all_files) == 1 and all_files[0].is_directory and len(ctx.files.nested_packages) == 0:
         # Special case where these is a single dep that is a directory artifact and there are no
@@ -188,7 +200,7 @@ def create_package(ctx, deps_files, nested_packages):
     args.use_param_file("%s", use_always = True)
     args.add(package_dir.path)
     args.add(package_path)
-    args.add_joined([s.path for s in ctx.files.srcs], join_with = ",", omit_if_empty = False)
+    args.add_joined([s.path for s in all_srcs], join_with = ",", omit_if_empty = False)
     args.add(ctx.bin_dir.path)
     args.add(ctx.genfiles_dir.path)
     args.add_joined(filtered_deps_sources, join_with = ",", omit_if_empty = False)
@@ -198,7 +210,11 @@ def create_package(ctx, deps_files, nested_packages):
     args.add(ctx.version_file.path if stamp else "")
     args.add_joined(ctx.attr.vendor_external, join_with = ",", omit_if_empty = False)
 
-    inputs = ctx.files.srcs + deps_files + nested_packages
+    validation_marker = []
+    for src in ctx.attr.srcs:
+        if _ValidationMarkerInfo in src:
+            validation_marker = [src[_ValidationMarkerInfo].marker]
+    inputs = all_srcs + deps_files + nested_packages + validation_marker
 
     # The version_file is an undocumented attribute of the ctx that lets us read the volatile-status.txt file
     # produced by the --workspace_status_command. That command will be executed whenever
@@ -242,9 +258,41 @@ def _create_npm_scripts(ctx, package_dir):
         execution_requirements = {"local": "1"},
     )
 
+def _validate_options_impl(ctx):
+    # Bazel won't run our action unless its output is needed, so make a marker file
+    # We make it a .d.ts file so we can plumb it to the deps of the ts_project compile.
+    marker = ctx.actions.declare_file("%s.optionsvalid.json" % ctx.label.name)
+
+    arguments = ctx.actions.args()
+    arguments.add_all([ctx.file.package_json.path, ctx.attr.package_name, ctx.attr.target, marker.path])
+
+    run_node(
+        ctx,
+        inputs = [ctx.file.package_json],
+        outputs = [marker],
+        arguments = [arguments],
+        executable = "validator",
+    )
+    return [
+        _ValidationMarkerInfo(marker = marker),
+    ]
+
+validate_options = rule(
+    implementation = _validate_options_impl,
+    attrs = {
+        "package_name": attr.string(),
+        "package_json": attr.label(mandatory = True, allow_single_file = [".json"]),
+        "target": attr.string(),
+        "validator": attr.label(default = Label("//internal/pkg_npm:validator"), executable = True, cfg = "host"),
+    },
+)
+
 def _pkg_npm(ctx):
     deps_files_depsets = []
 
+    if ctx.label.package + "/package.json" in [s.path for s in ctx.files.srcs]:
+        fail("package.json file should be supplied in the package_json attribute, not in srcs")
+
     for dep in ctx.attr.deps:
         # Collect whatever is in the "data"
         deps_files_depsets.append(dep.data_runfiles.files)
@@ -290,3 +338,34 @@ pkg_npm = rule(
     doc = _DOC,
     outputs = PKG_NPM_OUTPUTS,
 )
+
+def pkg_npm_macro(name, srcs = [], package_json = None, package_name = None, validate = True, **kwargs):
+    """
+    pkg_npm rule
+
+    Args:
+      srcs: e
+      package_json: the package.json that describes the package
+      **kwargs: e
+    """
+
+    srcs = srcs[:]  # defensive copy
+
+    # Adapt to the old API where package.json was just passed as one of the srcs
+    if "package.json" in srcs:
+        package_json = "package.json"
+        srcs.remove("package.json")
+
+    if not package_name:
+        package_name = name
+
+    if validate and package_json:
+        validate_options(
+            name = "_validate_%s" % name,
+            target = "//%s:%s" % (native.package_name(), name),
+            package_json = package_json,
+            package_name = package_name,
+        )
+        srcs.append("_validate_%s" % name)
+
+    pkg_npm(name = name, srcs = srcs, package_json = package_json, package_name = package_name, **kwargs)

internal/pkg_npm/pkg_npm_validator.js

@@ -0,0 +1,41 @@
+const fs = require('fs');
+
+function main([packageJsonPath, packageName, target, output]) {
+  const failures = [], buildozerCmds = [];
+  const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
+  if (packageJson.name !== packageName) {
+    failures.push(`attribute package_name=${packageName} does not match package.json name = "${
+        packageJson.name}"`);
+    buildozerCmds.push(`set package_name "${packageJson.name}"`)
+  }
+
+  if (failures.length > 0) {
+    console.error(`ERROR: pkg_npm rule ${
+        target} was configured with attributes that don't match the package.json file:`);
+    failures.forEach(f => console.error(' - ' + f));
+    console.error('\nYou can automatically fix this by running:');
+    console.error(
+        `    npx @bazel/buildozer ${buildozerCmds.map(c => `'${c}'`).join(' ')} ${target}`);
+    console.error('Or to suppress this error, run:');
+    console.error(`    npx @bazel/buildozer 'set validate False' ${target}`);
+    return 1;
+  }
+
+  // We have to write an output so that Bazel needs to execute this action.
+  // Make the output change whenever the attributes changed.
+  require('fs').writeFileSync(
+      output, `
+  // ${process.argv[1]} checked attributes for ${target}
+  // packageName:           ${packageName}
+  `,
+      'utf-8');
+  return 0;
+}
+
+if (require.main === module) {
+  try {
+    process.exitCode = main(process.argv.slice(2));
+  } catch (e) {
+    console.error(process.argv[1], e);
+  }
+}

internal/pkg_npm/test/BUILD.bazel

@@ -40,6 +40,7 @@ node_context_data(
 
 pkg_npm(
     name = "test_pkg",
+    package_name = "test-pkg",
     srcs = [
         "package.json",
         "some_file",

internal/pkg_npm/test/linking/bar/BUILD.bazel

@@ -19,5 +19,6 @@ pkg_npm(
         "main.js",
         "package.json",
     ],
+    validate = False,
     visibility = ["//internal/pkg_npm/test/linking:__pkg__"],
 )

internal/pkg_npm/test/linking/feb/BUILD.bazel

@@ -21,6 +21,7 @@ pkg_npm(
     name = "scoped_feb",
     package_name = "@scoped/feb",
     srcs = ["package.json"],
+    validate = False,
     visibility = ["//internal/pkg_npm/test/linking:__pkg__"],
     deps = [":feb_lib"],
 )

internal/pkg_npm/test/linking/fub/BUILD.bazel

@@ -32,6 +32,7 @@ pkg_npm(
     name = "scoped_fub",
     package_name = "@scoped/fub",
     srcs = ["package.json"],
+    validate = False,
     visibility = ["//internal/pkg_npm/test/linking:__pkg__"],
     deps = [":fub_lib"],
 )

packages/create/BUILD.bazel

@@ -16,6 +16,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/create",
     srcs = [
         "README.md",
         "index.js",

packages/karma/BUILD.bazel

@@ -105,6 +105,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/karma",
     srcs = [
         "index.bzl",
         "karma.conf.js",

packages/protractor/BUILD.bazel

@@ -85,6 +85,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/protractor",
     srcs = [
         "index.bzl",
         "package.bzl",

packages/rollup/BUILD.bazel

@@ -86,6 +86,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/rollup",
     srcs = [
         "index.bzl",
         "index.js",

packages/terser/BUILD.bazel

@@ -62,6 +62,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/terser",
     srcs = [
         "index.bzl",
         "index.js",

packages/typescript/BUILD.bazel

@@ -80,6 +80,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/typescript",
     srcs = [
         "index.bzl",
         "package.bzl",

packages/worker/BUILD.bazel

@@ -53,6 +53,7 @@ copy_file(
 
 pkg_npm(
     name = "npm_package",
+    package_name = "@bazel/worker",
     srcs = [
         "README.md",
         "package.json",

rules_typescript_pr_496.patch

@@ -2,10 +2,11 @@ diff BUILD.bazel BUILD.bazel
 index 3a519c5..6121ef6 100644
 --- BUILD.bazel
 +++ BUILD.bazel
-@@ -38,6 +38,7 @@ pkg_npm(
+@@ -38,6 +38,8 @@ pkg_npm(
          "//internal:npm_package_assets",
          "//third_party/github.com/bazelbuild/bazel/src/main/protobuf:npm_package_assets",
      ],
++    validate = False,
 +    vendor_external = ["build_bazel_rules_typescript"],
      visibility = ["//visibility:public"],
      deps = [