From ec5337ec7411c00c573b19a37be77b824770ab47 Mon Sep 17 00:00:00 2001
From: Alex Eagle <alex.eagle@robinhood.com>
Date: Thu, 10 Sep 2020 07:50:51 -0700
Subject: [PATCH] feat: promote js_library to public API

Fixes #149
---
 .github/workflows/stale.yml                   |   4 +-
 BUILD.bazel                                   |   1 +
 e2e/nodejs_image/foolib/BUILD.bazel           |   2 +-
 index.bzl                                     |   2 +
 index.for_docs.bzl                            |   2 +
 internal/js_library/js_library.bzl            | 130 ++++++++++++------
 .../dynamic_linked_pkg/BUILD.bazel            |   2 +-
 .../dynamic_linked_scoped_pkg/BUILD.bazel     |   2 +-
 .../integration/static_linked_pkg/BUILD.bazel |   2 +-
 .../static_linked_scoped_pkg/BUILD.bazel      |   2 +-
 .../transitive_static_linked_pkg/BUILD.bazel  |   2 +-
 internal/node/test/BUILD.bazel                |   2 +-
 internal/node/test/lib1/BUILD.bazel           |   2 +-
 internal/node/test/lib2/BUILD.bazel           |   2 +-
 internal/pkg_npm/test/linking/foo/BUILD.bazel |   2 +-
 internal/pkg_npm/test/linking/foz/BUILD.bazel |   2 +-
 .../rollup/test/integration/fum/BUILD.bazel   |   2 +-
 .../test/ts_project/js_library/BUILD.bazel    |   2 +-
 18 files changed, 110 insertions(+), 55 deletions(-)

diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml
index 7aa67be9a5..4a24fd293c 100644
--- a/.github/workflows/stale.yml
+++ b/.github/workflows/stale.yml
@@ -44,8 +44,8 @@ jobs:
 
         close-issue-message: >
           This issue was automatically closed because it went two weeks without a reply
-          since it was labelled "Can Close?"
+          since it was labeled "Can Close?"
 
         close-pr-message: >
           This PR was automatically closed because it went two weeks without a reply
-          since it was labelled "Can Close?"
+          since it was labeled "Can Close?"
diff --git a/BUILD.bazel b/BUILD.bazel
index 2736913c2b..40539f692b 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -49,6 +49,7 @@ bzl_library(
         "//internal/common:bzl",
         "//internal/generated_file_test:bzl",
         "//internal/linker:bzl",
+        "//internal/js_library:bzl",
         "//internal/pkg_npm:bzl",
         "//internal/pkg_web:bzl",
         "//internal/providers:bzl",
diff --git a/e2e/nodejs_image/foolib/BUILD.bazel b/e2e/nodejs_image/foolib/BUILD.bazel
index d005051922..f6cf8c1b04 100644
--- a/e2e/nodejs_image/foolib/BUILD.bazel
+++ b/e2e/nodejs_image/foolib/BUILD.bazel
@@ -1,4 +1,4 @@
-load("@build_bazel_rules_nodejs//internal/js_library:js_library.bzl", "js_library")
+load("@build_bazel_rules_nodejs//:index.bzl", "js_library")
 
 js_library(
     name = "foolib",
diff --git a/index.bzl b/index.bzl
index f0d456c558..ebe37f6ce0 100644
--- a/index.bzl
+++ b/index.bzl
@@ -23,6 +23,7 @@ load("//internal/common:check_version.bzl", "check_version")
 load("//internal/common:copy_to_bin.bzl", _copy_to_bin = "copy_to_bin")
 load("//internal/common:params_file.bzl", _params_file = "params_file")
 load("//internal/generated_file_test:generated_file_test.bzl", _generated_file_test = "generated_file_test")
+load("//internal/js_library:js_library.bzl", _js_library = "js_library")
 load(
     "//internal/node:node.bzl",
     _nodejs_binary = "nodejs_binary",
@@ -44,6 +45,7 @@ pkg_web = _pkg_web
 copy_to_bin = _copy_to_bin
 params_file = _params_file
 generated_file_test = _generated_file_test
+js_library = _js_library
 # ANY RULES ADDED HERE SHOULD BE DOCUMENTED, see index.for_docs.bzl
 
 # Allows us to avoid a transitive dependency on bazel_skylib from leaking to users
diff --git a/index.for_docs.bzl b/index.for_docs.bzl
index 9b4a7dd21e..37930e652e 100644
--- a/index.for_docs.bzl
+++ b/index.for_docs.bzl
@@ -20,6 +20,7 @@ load("//internal/common:check_bazel_version.bzl", _check_bazel_version = "check_
 load("//internal/common:copy_to_bin.bzl", _copy_to_bin = "copy_to_bin")
 load("//internal/common:params_file.bzl", _params_file = "params_file")
 load("//internal/generated_file_test:generated_file_test.bzl", _generated_file_test = "generated_file_test")
+load("//internal/js_library:js_library.bzl", _js_library = "js_library")
 load("//internal/node:node.bzl", _nodejs_binary = "nodejs_binary", _nodejs_test = "nodejs_test")
 load("//internal/node:node_repositories.bzl", _node_repositories = "node_repositories_rule")
 load("//internal/node:npm_package_bin.bzl", _npm_bin = "npm_package_bin")
@@ -39,4 +40,5 @@ yarn_install = _yarn_install
 npm_package_bin = _npm_bin
 pkg_web = _pkg_web
 generated_file_test = _generated_file_test
+js_library = _js_library
 # ANY RULES ADDED HERE SHOULD BE DOCUMENTED, run yarn stardoc to verify
diff --git a/internal/js_library/js_library.bzl b/internal/js_library/js_library.bzl
index e023fd2653..24c964f6b7 100644
--- a/internal/js_library/js_library.bzl
+++ b/internal/js_library/js_library.bzl
@@ -37,6 +37,41 @@ load(
 _AMD_NAMES_DOC = """Mapping from require module names to global variables.
 This allows devmode JS sources to load unnamed UMD bundles from third-party libraries."""
 
+_ATTRS = {
+    "amd_names": attr.string_dict(
+        doc = _AMD_NAMES_DOC,
+    ),
+    "deps": attr.label_list(
+        doc = """Transitive dependencies of the package.
+        It should include fine grained npm dependencies from the sources
+        or other targets we want to include in the library but also propagate their own deps.""",
+    ),
+    "is_windows": attr.bool(
+        doc = "Automatically set by macro",
+        mandatory = True,
+    ),
+    # module_name for legacy ts_library module_mapping support
+    # which is still being used in a couple of tests
+    # TODO: remove once legacy module_mapping is removed
+    "module_name": attr.string(
+        doc = "Internal use only. It will be removed soon.",
+    ),
+    "named_module_srcs": attr.label_list(
+        doc = """A subset of srcs that are javascript named-UMD or
+        named-AMD for use in rules such as ts_devserver.
+        They will be copied into the package bin folder if needed.""",
+        allow_files = True,
+    ),
+    "package_name": attr.string(
+        doc = """Optional package_name that this package may be imported as.""",
+    ),
+    "srcs": attr.label_list(
+        doc = """The list of files that comprise the package.
+        They will be copied into the package bin folder if needed.""",
+        allow_files = True,
+    ),
+}
+
 AmdNamesInfo = provider(
     doc = "provide access to the amd_names attribute of js_library",
     fields = {"names": _AMD_NAMES_DOC},
@@ -48,7 +83,7 @@ def write_amd_names_shim(actions, amd_names_shim, targets):
     These are collected from our bootstrap deps (the only place global scripts should appear)
 
     Args:
-      actions: skylark rule execution context.actions
+      actions: starlark rule execution context.actions
       amd_names_shim: File where the shim is written
       targets: dependencies to be scanned for AmdNamesInfo providers
     """
@@ -199,57 +234,72 @@ def _impl(ctx):
 
 _js_library = rule(
     implementation = _impl,
-    attrs = {
-        "amd_names": attr.string_dict(
-            doc = _AMD_NAMES_DOC,
-        ),
-        "deps": attr.label_list(
-            doc = """Transitive dependencies of the package.
-            It should include fine grained npm dependencies from the sources
-            or other targets we want to include in the library but also propagate their own deps.""",
-        ),
-        "is_windows": attr.bool(
-            doc = "Automatically set by macro",
-            mandatory = True,
-        ),
-        # module_name for legacy ts_library module_mapping support
-        # which is still being used in a couple of tests
-        # TODO: remove once legacy module_mapping is removed
-        "module_name": attr.string(
-            doc = "Internal use only. It will be removed soon.",
-        ),
-        "named_module_srcs": attr.label_list(
-            doc = """A subset of srcs that are javascript named-UMD or
-            named-AMD for use in rules such as ts_devserver.
-            They will be copied into the package bin folder if needed.""",
-            allow_files = True,
-        ),
-        "package_name": attr.string(
-            doc = """Optional package_name that this package may be imported as.""",
-        ),
-        "srcs": attr.label_list(
-            doc = """The list of files that comprise the package.
-            They will be copied into the package bin folder if needed.""",
-            allow_files = True,
-        ),
-    },
-    doc = "Defines a js_library package",
+    attrs = _ATTRS,
 )
 
 def js_library(
         name,
         srcs = [],
-        amd_names = {},
         package_name = None,
         deps = [],
-        named_module_srcs = [],
         **kwargs):
-    """Internal use only yet. It will be released into a public API in a future release."""
+    """Groups JavaScript code so that it can be depended on like an npm package.
+
+    This rule doesn't perform any build steps ("actions") so it is similar to a `filegroup`.
+    However it produces several Bazel "Providers" for interop with other rules.
+    
+    > Compare this to `pkg_npm` which just produces a directory output, and therefore can't expose individual
+    > files to downstream targets and causes a cascading re-build of all transitive dependencies when any file
+    > changes
+    
+    These providers are:
+    - DeclarationInfo so this target can be a dependency of a TypeScript rule
+    - NpmPackageInfo so this target can interop with rules that expect third-party npm packages
+    - LinkablePackageInfo for use with our "linker" that makes this package importable (similar to `npm link`)
+    - JsModuleInfo so rules like bundlers can collect the transitive set of .js files
+    
+    `js_library` is intended to be used internally within Bazel, such as between two libraries in your monorepo.
+    
+    > Compare this to `pkg_npm` which is intended to publish your code for external usage outside of Bazel, like
+    > by publishing to npm or artifactory.
+
+    The typical example usage of `js_library` is to expose some sources with a package name:
+
+    ```python
+    ts_project(
+        srcs = glob(["*.ts"]),
+    )
+
+    js_library(
+        name = "my_pkg",
+        # Code that depends on this target can import from "@myco/mypkg"
+        package_name = "@myco/mypkg",
+        deps = [":tsconfig"],
+    )
+    ```
+
+    To help work with "named AMD" modules as required by `ts_devserver` and other Google-style "concatjs" rules,
+    `js_library` has some undocumented advanced features you can find in the source code or in our examples.
+    These should not be considered a public API and aren't subject to our usual support and semver guarantees.
+
+    Args:
+        name: a name for the target
+        srcs: the list of files that comprise the package
+        package_name: the name it will be imported by. Should match the "name" field in the package.json file.
+        deps: other targets that provide JavaScript code
+        **kwargs: used for undocumented legacy features
+    """
+
+    # Undocumented features
+    amd_names = kwargs.pop("amd_names", {})
     module_name = kwargs.pop("module_name", None)
+    named_module_srcs = kwargs.pop("named_module_srcs", [])
+
     if module_name:
         fail("use package_name instead of module_name in target //%s:%s" % (native.package_name(), name))
     if kwargs.pop("is_windows", None):
-        fail("is_windows is set by the js_library macro and should not be set explicitely")
+        fail("is_windows is set by the js_library macro and should not be set explicitly")
+
     _js_library(
         name = name,
         amd_names = amd_names,
diff --git a/internal/linker/test/integration/dynamic_linked_pkg/BUILD.bazel b/internal/linker/test/integration/dynamic_linked_pkg/BUILD.bazel
index 593f1f3d52..e2a0663e1f 100644
--- a/internal/linker/test/integration/dynamic_linked_pkg/BUILD.bazel
+++ b/internal/linker/test/integration/dynamic_linked_pkg/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//internal/linker/test:__subpackages__"])
 
diff --git a/internal/linker/test/integration/dynamic_linked_scoped_pkg/BUILD.bazel b/internal/linker/test/integration/dynamic_linked_scoped_pkg/BUILD.bazel
index d1c2df517a..dbc3be1d99 100644
--- a/internal/linker/test/integration/dynamic_linked_scoped_pkg/BUILD.bazel
+++ b/internal/linker/test/integration/dynamic_linked_scoped_pkg/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//internal/linker/test:__subpackages__"])
 
diff --git a/internal/linker/test/integration/static_linked_pkg/BUILD.bazel b/internal/linker/test/integration/static_linked_pkg/BUILD.bazel
index 4872e50a56..49db812a32 100644
--- a/internal/linker/test/integration/static_linked_pkg/BUILD.bazel
+++ b/internal/linker/test/integration/static_linked_pkg/BUILD.bazel
@@ -1,5 +1,5 @@
 load("@build_bazel_rules_nodejs//:index.bzl", "copy_to_bin")
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//internal/linker/test:__subpackages__"])
 
diff --git a/internal/linker/test/integration/static_linked_scoped_pkg/BUILD.bazel b/internal/linker/test/integration/static_linked_scoped_pkg/BUILD.bazel
index 894a1d632d..8b34fd2202 100644
--- a/internal/linker/test/integration/static_linked_scoped_pkg/BUILD.bazel
+++ b/internal/linker/test/integration/static_linked_scoped_pkg/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//internal/linker/test:__subpackages__"])
 
diff --git a/internal/linker/test/integration/transitive_static_linked_pkg/BUILD.bazel b/internal/linker/test/integration/transitive_static_linked_pkg/BUILD.bazel
index 51e4a7df9c..28b85b1ebd 100644
--- a/internal/linker/test/integration/transitive_static_linked_pkg/BUILD.bazel
+++ b/internal/linker/test/integration/transitive_static_linked_pkg/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//internal/linker/test:__subpackages__"])
 
diff --git a/internal/node/test/BUILD.bazel b/internal/node/test/BUILD.bazel
index e23d3d5fc0..6c1d0398a3 100644
--- a/internal/node/test/BUILD.bazel
+++ b/internal/node/test/BUILD.bazel
@@ -1,6 +1,6 @@
 load("@build_bazel_rules_nodejs//:index.bzl", "generated_file_test", "nodejs_binary", "nodejs_test", "npm_package_bin")
 load("@npm//typescript:index.bzl", "tsc")
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 load("//internal/node:node_repositories.bzl", "BUILT_IN_NODE_PLATFORMS")
 load("//packages/jasmine:index.bzl", "jasmine_node_test")
 load("//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_file.bzl", "copy_file")
diff --git a/internal/node/test/lib1/BUILD.bazel b/internal/node/test/lib1/BUILD.bazel
index 25632d351e..0e54cda27b 100644
--- a/internal/node/test/lib1/BUILD.bazel
+++ b/internal/node/test/lib1/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//internal:__subpackages__"])
 
diff --git a/internal/node/test/lib2/BUILD.bazel b/internal/node/test/lib2/BUILD.bazel
index 796c867d6e..6b14af8a69 100644
--- a/internal/node/test/lib2/BUILD.bazel
+++ b/internal/node/test/lib2/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 load("//packages/typescript:index.bzl", "ts_project")
 
 package(default_visibility = ["//internal:__subpackages__"])
diff --git a/internal/pkg_npm/test/linking/foo/BUILD.bazel b/internal/pkg_npm/test/linking/foo/BUILD.bazel
index ae2ee734aa..560eb83c1f 100644
--- a/internal/pkg_npm/test/linking/foo/BUILD.bazel
+++ b/internal/pkg_npm/test/linking/foo/BUILD.bazel
@@ -1,5 +1,5 @@
 load("@build_bazel_rules_nodejs//:index.bzl", "copy_to_bin", "pkg_npm")
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 copy_to_bin(
     name = "foo_copy_to_bin",
diff --git a/internal/pkg_npm/test/linking/foz/BUILD.bazel b/internal/pkg_npm/test/linking/foz/BUILD.bazel
index bc10566b88..c27d03ec35 100644
--- a/internal/pkg_npm/test/linking/foz/BUILD.bazel
+++ b/internal/pkg_npm/test/linking/foz/BUILD.bazel
@@ -1,5 +1,5 @@
 load("@build_bazel_rules_nodejs//:index.bzl", "copy_to_bin", "pkg_npm")
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 copy_to_bin(
     name = "foz_copy_to_bin",
diff --git a/packages/rollup/test/integration/fum/BUILD.bazel b/packages/rollup/test/integration/fum/BUILD.bazel
index b1d9ebbe16..d37f63d8df 100644
--- a/packages/rollup/test/integration/fum/BUILD.bazel
+++ b/packages/rollup/test/integration/fum/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 
 package(default_visibility = ["//packages/rollup:__subpackages__"])
 
diff --git a/packages/typescript/test/ts_project/js_library/BUILD.bazel b/packages/typescript/test/ts_project/js_library/BUILD.bazel
index b13c485b91..6edab8cd20 100644
--- a/packages/typescript/test/ts_project/js_library/BUILD.bazel
+++ b/packages/typescript/test/ts_project/js_library/BUILD.bazel
@@ -1,4 +1,4 @@
-load("//internal/js_library:js_library.bzl", "js_library")
+load("//:index.bzl", "js_library")
 load("//packages/typescript:index.bzl", "ts_project")
 
 js_library(