Skip to content

javascript: Do not read none mapped package.json files in javascript rules #18523

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

Merged
stuhood merged 9 commits into from
Apr 3, 2023
Merged

javascript: Do not read none mapped package.json files in javascript rules #18523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
529487a
refactor: Split rule into logical parts
tobni Mar 17, 2023
0c46b69
refactor: Extract rule for resolving target generation requests
tobni Mar 18, 2023
4b8c121
refactor: Reduce indentation
tobni Mar 18, 2023
c1bc136
fix: Use resolved target type everwhere
tobni Mar 18, 2023
9806d80
refactor: Make address families easier to reach
tobni Mar 18, 2023
010dece
fix: Only parse package_json that are mapped
tobni Mar 18, 2023
0b36a0e
refactor: Switch overrides calc to rule helper
tobni Apr 3, 2023
4699e7c
doc: Add comment for workaround
tobni Apr 3, 2023
4d0defd
fix: Move rule helper above consuming rule
tobni Apr 3, 2023
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
3 changes: 3 additions & 0 deletions src/python/pants/backend/javascript/nodejs_project_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ def test_parses_project_with_workspaces(rule_runner: RuleRunner) -> None:
rule_runner.write_files(
{
"src/js/package.json": given_package_with_workspaces("egg", "1.0.0", "foo", "bar"),
"src/js/BUILD": "package_json()",
"src/js/foo/BUILD": "package_json()",
"src/js/foo/package.json": given_package("ham", "0.0.1"),
"src/js/bar/BUILD": "package_json()",
Expand All @@ -81,6 +82,7 @@ def test_parses_project_with_nested_workspaces(rule_runner: RuleRunner) -> None:
rule_runner.write_files(
{
"src/js/package.json": given_package_with_workspaces("egg", "1.0.0", "foo"),
"src/js/BUILD": "package_json()",
"src/js/foo/BUILD": "package_json()",
"src/js/foo/package.json": given_package_with_workspaces("ham", "0.0.1", "bar"),
"src/js/foo/bar/BUILD": "package_json()",
Expand All @@ -96,6 +98,7 @@ def test_workspaces_with_multiple_owners_is_an_error(rule_runner: RuleRunner) ->
rule_runner.write_files(
{
"src/js/package.json": given_package_with_workspaces("egg", "1.0.0", "foo/bar"),
"src/js/BUILD": "package_json()",
"src/js/foo/BUILD": "package_json()",
"src/js/foo/package.json": given_package_with_workspaces("ham", "0.0.1", "bar"),
"src/js/foo/bar/BUILD": "package_json()",
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/backend/javascript/package/rules_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def test_packages_files_as_resource_in_workspace(rule_runner: RuleRunner) -> Non
"src/js/package.json": json.dumps(
{"name": "spam", "version": "0.0.1", "workspaces": ["a"]}
),
"src/js/BUILD": "package_json()",
"src/js/a/BUILD": dedent(
"""\
package_json(
Expand Down
30 changes: 29 additions & 1 deletion src/python/pants/backend/javascript/package_json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
from pants.engine.collection import Collection, DeduplicatedCollection
from pants.engine.fs import DigestContents, FileContent, GlobExpansionConjunction, PathGlobs
from pants.engine.internals import graph
from pants.engine.internals.graph import (
ResolveAllTargetGeneratorRequests,
ResolvedTargetGeneratorRequests,
)
from pants.engine.internals.native_engine import Digest, Snapshot
from pants.engine.internals.selectors import Get, MultiGet
from pants.engine.rules import Rule, collect_rules, rule
Expand Down Expand Up @@ -621,7 +625,31 @@ async def read_package_jsons(globs: PathGlobs) -> PackageJsonForGlobs:

@rule
async def all_package_json() -> AllPackageJson:
Copy link
Member

@stuhood stuhood Apr 3, 2023

Choose a reason for hiding this comment

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

It would be good to add a note here that explains why this avoids using the more obvious APIs.

return AllPackageJson(await Get(PackageJsonForGlobs, PathGlobs(["**/package.json"])))
# Avoids using `AllTargets` due to a circular rule dependency.
# `generate_node_package_targets` requires knowledge of all
# first party package names.
description_of_origin = "The `AllPackageJson` rule"
requests = await Get(
ResolvedTargetGeneratorRequests,
ResolveAllTargetGeneratorRequests(
description_of_origin=description_of_origin, of_type=PackageJsonTarget
),
)
globs = [
glob
for req in requests.requests
for glob in req.generator[PackageJsonSourceField]
.path_globs(UnmatchedBuildFileGlobs.error())
.globs
]
return AllPackageJson(
await Get(
PackageJsonForGlobs,
PathGlobs(
globs, GlobMatchErrorBehavior.error, description_of_origin=description_of_origin
),
)
)


class AllPackageJsonNames(DeduplicatedCollection[str]):
Expand Down
19 changes: 19 additions & 0 deletions src/python/pants/backend/javascript/package_json_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,25 @@ def test_does_not_generate_third_party_node_package_target_for_first_party_packa
]


def test_does_not_consider_package_json_without_a_target(
rule_runner: RuleRunner,
) -> None:
rule_runner.write_files(
{
"src/js/a/BUILD": "package_json()",
"src/js/a/package.json": json.dumps(
{"name": "ham", "version": "0.0.1", "dependencies": {"chalk": "^5.2.0"}}
),
"src/js/b/package.json": json.dumps(
{"name": "spam", "version": "0.0.1", "dependencies": {"ham": "0.0.1"}}
),
}
)
all_packages = rule_runner.request(AllPackageJson, ())
assert len(all_packages) == 1
assert all_packages[0].name == "ham"


def test_generates_build_script_targets(
rule_runner: RuleRunner,
) -> None:
Expand Down
Loading