Skip to content

feat: add prod and dev attributes to npm_link_targets #2051

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: add prod and dev attributes to npm_link_targets #2051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion npm/extensions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ WARNING: Cannot determine home directory in order to load home `.npmrc` file in
system_tar = detect_system_tar(module_ctx)

for i in imports:
link_packages = {}
for link_package, link_names in i.link_packages.items():
link_packages[link_package] = [link_name["pkg"] for link_name in link_names]
npm_import(
name = i.name,
bins = i.bins,
Expand All @@ -149,7 +152,7 @@ WARNING: Cannot determine home directory in order to load home `.npmrc` file in
lifecycle_hooks_env = i.lifecycle_hooks_env,
lifecycle_hooks_execution_requirements = i.lifecycle_hooks_execution_requirements,
lifecycle_hooks_use_default_shell_env = i.lifecycle_hooks_use_default_shell_env,
link_packages = i.link_packages,
link_packages = link_packages,
# attr.pnpm_lock.repo_name is a canonical repository name, so it needs to be qualified with an extra '@'.
link_workspace = attr.link_workspace if attr.link_workspace else "@" + attr.pnpm_lock.repo_name,
npm_auth = i.npm_auth,
Expand Down
4 changes: 2 additions & 2 deletions npm/private/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ exports_files(["noop.sh"])
"npm_translate_lock_generate.docs.bzl",
])
for j, expected in enumerate([
"""def npm_link_targets(name = "node_modules", package = None):""",
"""def npm_link_all_packages(name = "node_modules", imported_links = []):""",
"""def npm_link_targets(name = "node_modules", package = None, prod = False, dev = False):""",
"""def npm_link_all_packages(name = "node_modules", imported_links = [], prod = False, dev = False):""",
])
]

Expand Down
168 changes: 89 additions & 79 deletions npm/private/npm_translate_lock_generate.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ _FP_DIRECT_TMPL = \

_FP_DIRECT_TARGET_TMPL = \
"""
if bazel_package in {link_packages}:
if bazel_package in {link_packages} and ((not prod and not dev) or {prod_or_dev}):
link_targets.append(":{{}}/{pkg}".format(name))"""

_BZL_LIBRARY_TMPL = \
Expand Down Expand Up @@ -151,62 +151,66 @@ sh_binary(
"package": name,
"path": dep_path,
"link_packages": {},
"link_dev_packages": {},
"deps": transitive_deps,
}

# Look for first-party links in importers
for import_path, importer in importers.items():
dependencies = importer.get("all_deps")
if type(dependencies) != "dict":
msg = "expected dict of dependencies in processed importer '{}'".format(import_path)
fail(msg)
link_package = helpers.link_package(root_package, import_path)
for dep_package, dep_version in dependencies.items():
if dep_version.startswith("file:"):
dep_key = "{}+{}".format(dep_package, dep_version)
if not dep_key in fp_links.keys():
msg = "Expected to file: referenced package {} in first-party links {}".format(dep_key, fp_links.keys())
fail(msg)
fp_links[dep_key]["link_packages"][link_package] = True
elif dep_version.startswith("link:"):
dep_link = dep_version[len("link:"):]
dep_path = helpers.link_package(root_package, dep_link)
dep_key = "{}+{}".format(dep_package, dep_path)
if fp_links.get(dep_key, False):
fp_links[dep_key]["link_packages"][link_package] = True
else:
transitive_deps = {}
raw_deps = {}
if importers.get(dep_link, False):
raw_deps = importers.get(dep_link).get("deps")
for raw_package, raw_version in raw_deps.items():
package_store_name = utils.package_store_name(raw_package, raw_version)
dep_store_target = """"//{root_package}:{package_store_root}/{{}}/{package_store_name}".format(name)""".format(
root_package = root_package,
package_store_name = package_store_name,
package_store_root = utils.package_store_root,
)
if dep_store_target not in transitive_deps:
transitive_deps[dep_store_target] = [raw_package]
else:
transitive_deps[dep_store_target].append(raw_package)

# collapse link aliases lists into to a comma separated strings
for dep_store_target in transitive_deps.keys():
transitive_deps[dep_store_target] = ",".join(transitive_deps[dep_store_target])
fp_links[dep_key] = {
"package": dep_package,
"path": dep_path,
"link_packages": {link_package: True},
"deps": transitive_deps,
}
for deps_type, link_type in [("deps", "link_packages"), ("dev_deps", "link_dev_packages")]:
dependencies = importer.get(deps_type)
if type(dependencies) != "dict":
msg = "expected dict of dependencies in processed importer '{}'".format(import_path)
fail(msg)
for dep_package, dep_version in dependencies.items():
if dep_version.startswith("file:"):
dep_key = "{}+{}".format(dep_package, dep_version)
if not dep_key in fp_links.keys():
msg = "Expected to file: referenced package {} in first-party links {}".format(dep_key, fp_links.keys())
fail(msg)
fp_links[dep_key][link_type][link_package] = True
elif dep_version.startswith("link:"):
dep_link = dep_version[len("link:"):]
dep_path = helpers.link_package(root_package, dep_link)
dep_key = "{}+{}".format(dep_package, dep_path)
if not fp_links.get(dep_key, False):
transitive_deps = {}
raw_deps = {}
if importers.get(dep_link, False):
raw_deps = importers.get(dep_link).get("deps")
for raw_package, raw_version in raw_deps.items():
package_store_name = utils.package_store_name(raw_package, raw_version)
dep_store_target = """"//{root_package}:{package_store_root}/{{}}/{package_store_name}".format(name)""".format(
root_package = root_package,
package_store_name = package_store_name,
package_store_root = utils.package_store_root,
)
if dep_store_target not in transitive_deps:
transitive_deps[dep_store_target] = [raw_package]
else:
transitive_deps[dep_store_target].append(raw_package)

# collapse link aliases lists into to a comma separated strings
for dep_store_target in transitive_deps.keys():
transitive_deps[dep_store_target] = ",".join(transitive_deps[dep_store_target])
fp_links[dep_key] = {
"package": dep_package,
"path": dep_path,
"link_packages": {},
"link_dev_packages": {},
"deps": transitive_deps,
}
fp_links[dep_key][link_type][link_package] = True

npm_link_packages_const = """_LINK_PACKAGES = {link_packages}""".format(link_packages = str(link_packages))

npm_link_targets_bzl = [
"""\
# buildifier: disable=function-docstring
def npm_link_targets(name = "node_modules", package = None):
def npm_link_targets(name = "node_modules", package = None, prod = False, dev = False):
if prod and dev:
fail("prod and dev attributes cannot both be set to true")
bazel_package = package if package != None else native.package_name()
link = bazel_package in _LINK_PACKAGES

Expand All @@ -217,7 +221,7 @@ def npm_link_targets(name = "node_modules", package = None):
npm_link_all_packages_bzl = [
"""\
# buildifier: disable=function-docstring
def npm_link_all_packages(name = "node_modules", imported_links = []):
def npm_link_all_packages(name = "node_modules", imported_links = [], prod = False, dev = False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other ideas for this API?

It might be nice if we didn't have this scenario where 1/4 of the states is basically invalid and has an odd non-intuitive default. Maybe just throw when false/false and then the other 3 states are valid and understandable?

dev = True|False|None or something like that is one alternative, although the word "dev" in the API without "prod" in the API is confusing imo. Or type = "dev"|"prod"|None but I'd want a term better then "type".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually is this API simply a copy of npm_translate_lock? In which case we should do exactly as you have and my suggestion can be considered in rules_js v3 or something like that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's just following the prod/dev API from npm_translate_lock. I agree that an API that excludes invalid states by design would be better for v3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current API does not allow a single BUILD to have a mix of these though, correct? If you wanted to include dev deps for some targets (such as tests) but not for others (such as libraries).

Would something such as :node_modules[__{dev,prod}] as the API also solve this?

Copy link
Contributor Author

@jfirebaugh jfirebaugh Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does allow a mix. You could write npm_link_targets() and npm_link_targets(prod = True) and npm_link_targets(dev = True) in the same BUILD if you wanted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking you can't have multiple npm_link_all_packages() targets though...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the npm_link_all_packages API changes in this PR at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why I included the npm_link_all_packages changes originally; I guess just for parallelism with npm_link_targets. We're only using npm_link_targets(prod = True).

bazel_package = native.package_name()
root_package = "{root_package}"
is_root = bazel_package == root_package
Expand Down Expand Up @@ -266,9 +270,7 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
)

stores_bzl.append(""" store_{i}(name)""".format(i = i))
for link_package, _link_aliases in _import.link_packages.items():
link_aliases = _link_aliases or [_import.package]

for link_package, link_names in _import.link_packages.items():
# the build file for the package being linked
build_file = "{}/{}".format(link_package, "BUILD.bazel") if link_package else "BUILD.bazel"
if build_file not in rctx_files:
Expand All @@ -281,7 +283,10 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
links_targets_bzl[link_package] = []

# for each alias of this package
for link_alias in link_aliases:
for link_name in link_names:
link_alias = link_name["pkg"]
dev = link_name["dev"]

# link the alias to the underlying package
links_bzl[link_package].append(""" link_{i}("{{}}/{alias}".format(name), link_root_name = name, link_alias = "{alias}")""".format(
i = i,
Expand All @@ -290,7 +295,7 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):

# expose the alias if public
if "//visibility:public" in _import.package_visibility:
add_to_link_targets = """ link_targets.append(":{{}}/{alias}".format(name))""".format(alias = link_alias)
add_to_link_targets = """ link_targets.append(":{{}}/{alias}".format(name)) if (not prod and not dev) or {prod_or_dev} else None""".format(alias = link_alias, prod_or_dev = "dev" if dev else "prod")
links_bzl[link_package].append(add_to_link_targets)
links_targets_bzl[link_package].append(add_to_link_targets)
package_scope = link_alias[:link_alias.find("/", 1)] if link_alias[0] == "@" else None
Expand All @@ -314,19 +319,19 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
if rctx.attr.generate_bzl_library_targets:
rctx_files[build_file].append("""load("@bazel_skylib//:bzl_library.bzl", "bzl_library")""")

for link_alias in link_aliases:
for link_name in link_names:
rctx_files[build_file].append(_BZL_LIBRARY_TMPL.format(
name = link_alias,
src = ":{}/{}".format(link_alias, _PACKAGE_JSON_BZL_FILENAME),
name = link_name["pkg"],
src = ":{}/{}".format(link_name["pkg"], _PACKAGE_JSON_BZL_FILENAME),
dep = "@{repo_name}//{link_package}:{package_name}_bzl_library".format(
repo_name = helpers.to_apparent_repo_name(_import.name),
link_package = link_package,
package_name = link_package[link_package.rfind("/") + 1] if link_package else link_alias.split("/")[-1],
package_name = link_package[link_package.rfind("/") + 1] if link_package else link_name["pkg"].split("/")[-1],
),
))

for link_alias in link_aliases:
package_json_bzl_file_path = "{}/{}/{}".format(link_package, link_alias, _PACKAGE_JSON_BZL_FILENAME) if link_package else "{}/{}".format(link_alias, _PACKAGE_JSON_BZL_FILENAME)
for link_name in link_names:
package_json_bzl_file_path = "{}/{}/{}".format(link_package, link_name["pkg"], _PACKAGE_JSON_BZL_FILENAME) if link_package else "{}/{}".format(link_name["pkg"], _PACKAGE_JSON_BZL_FILENAME)
repo_package_json_bzl = "@@{repo_name}//{link_package}:{package_json_bzl}".format(
repo_name = _import.name,
link_package = link_package,
Expand Down Expand Up @@ -368,7 +373,6 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
for fp_link in fp_links.values():
fp_package = fp_link.get("package")
fp_path = fp_link.get("path")
fp_link_packages = fp_link.get("link_packages").keys()
fp_deps = fp_link.get("deps")
fp_target = "//{}:{}".format(
fp_path,
Expand All @@ -387,28 +391,31 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
if len(package_visibility) == 0:
package_visibility = ["//visibility:public"]

if len(fp_link_packages) > 0:
npm_link_all_packages_bzl.append(_FP_DIRECT_TMPL.format(
link_packages = fp_link_packages,
link_visibility = package_visibility,
pkg = fp_package,
package_directory_output_group = utils.package_directory_output_group,
root_package = root_package,
package_store_name = utils.package_store_name(fp_package, "0.0.0"),
package_store_root = utils.package_store_root,
))
for link_type in ["link_packages", "link_dev_packages"]:
fp_link_packages = fp_link.get(link_type).keys()
if len(fp_link_packages) > 0:
npm_link_all_packages_bzl.append(_FP_DIRECT_TMPL.format(
link_packages = fp_link_packages,
link_visibility = package_visibility,
pkg = fp_package,
package_directory_output_group = utils.package_directory_output_group,
root_package = root_package,
package_store_name = utils.package_store_name(fp_package, "0.0.0"),
package_store_root = utils.package_store_root,
))

npm_link_targets_bzl.append(_FP_DIRECT_TARGET_TMPL.format(
link_packages = fp_link_packages,
pkg = fp_package,
))
npm_link_targets_bzl.append(_FP_DIRECT_TARGET_TMPL.format(
link_packages = fp_link_packages,
pkg = fp_package,
prod_or_dev = "dev" if link_type == "link_dev_packages" else "prod",
))

if "//visibility:public" in package_visibility:
add_to_link_targets = """ link_targets.append(":{{}}/{pkg}".format(name))""".format(pkg = fp_package)
npm_link_all_packages_bzl.append(add_to_link_targets)
package_scope = fp_package[:fp_package.find("/", 1)] if fp_package[0] == "@" else None
if package_scope:
npm_link_all_packages_bzl.append(_ADD_SCOPE_TARGET2.format(package_scope = package_scope))
if "//visibility:public" in package_visibility:
add_to_link_targets = """ link_targets.append(":{{}}/{pkg}".format(name))""".format(pkg = fp_package)
npm_link_all_packages_bzl.append(add_to_link_targets)
package_scope = fp_package[:fp_package.find("/", 1)] if fp_package[0] == "@" else None
if package_scope:
npm_link_all_packages_bzl.append(_ADD_SCOPE_TARGET2.format(package_scope = package_scope))

# Generate catch all & scoped js_library targets
npm_link_all_packages_bzl.append("""
Expand Down Expand Up @@ -543,8 +550,11 @@ def _gen_npm_import(rctx, system_tar, _import, link_workspace):
maybe_exclude_package_contents = ("""
exclude_package_contents = %s,""" % _import.exclude_package_contents) if _import.exclude_package_contents != None else ""

link_packages = {}
for link_package, link_names in _import.link_packages.items():
link_packages[link_package] = [link_name["pkg"] for link_name in link_names]
return _NPM_IMPORT_TMPL.format(
link_packages = starlark_codegen_utils.to_dict_attr(_import.link_packages, 2, quote_value = False),
link_packages = starlark_codegen_utils.to_dict_attr(link_packages, 2, quote_value = False),
link_workspace = link_workspace,
maybe_bins = maybe_bins,
maybe_commit = maybe_commit,
Expand Down
8 changes: 6 additions & 2 deletions npm/private/npm_translate_lock_generate.docs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ load("@npm//:defs.bzl", "npm_link_targets", "npm_link_all_packages")
# Instead, we define the interfaces in this file, while the implementations live in npm_translate_lock_generate.bzl.

# buildifier: disable=unused-variable
def npm_link_targets(name = "node_modules", package = None):
def npm_link_targets(name = "node_modules", package = None, prod = False, dev = False):
"""Generated list of target names that are linked by npm_link_all_packages()

Args:
Expand All @@ -27,14 +27,16 @@ def npm_link_targets(name = "node_modules", package = None):
Set to an empty string "" to specify the root package.

If unspecified, the current package (`native.package_name()`) is used.
prod: when True, only include production dependencies
dev: when True, only include development dependencies

Returns:
A list of target names that are linked by npm_link_all_packages()
"""
pass

# buildifier: disable=unused-variable
def npm_link_all_packages(name = "node_modules", imported_links = []):
def npm_link_all_packages(name = "node_modules", imported_links = [], prod = False, dev = False):
"""Generated list of npm_link_package() target generators and first-party linked packages corresponding to the packages in {pnpm_lock_label}

If you use manually-written [`npm_import`](/docs/npm_import.md#npm_import) you can link these as well, for example,
Expand All @@ -52,5 +54,7 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
Args:
name: name of catch all target to generate for all packages linked
imported_links: optional list link functions from manually imported packages that were fetched with npm_import rules.
prod: when True, only include production dependencies
dev: when True, only include development dependencies
"""
pass
Loading
Loading