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.

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

Add package support to typescript/javascript dependency inference #21556

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

Add package support to typescript/javascript dependency inference #21556

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
81 changes: 55 additions & 26 deletions src/python/pants/backend/javascript/dependency_inference/rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,13 @@ def _create_inference_metadata(
)


async def _prepare_inference_metadata(address: Address, file_path: str) -> InferenceMetadata:
owning_pkg, maybe_config = await concurrently(
find_owning_package(OwningNodePackageRequest(address)),
find_parent_ts_config(ParentTSConfigRequest(file_path, "jsconfig.json"), **implicitly()),
)
async def _prepare_inference_metadata(owning_pkg, maybe_config, spec_path) -> InferenceMetadata:
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to ensure there's type annotations on these args if possible.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yep, meant to get back around to that and never did...

if not owning_pkg.target:
return InferenceMetadata.javascript(
(
os.path.dirname(maybe_config.ts_config.path)
if maybe_config.ts_config
else address.spec_path
else spec_path
),
{},
maybe_config.ts_config.resolution_root_dir if maybe_config.ts_config else None,
Expand Down Expand Up @@ -173,29 +169,57 @@ def _add_extensions(file_imports: frozenset[str], file_extensions: tuple[str, ..
async def _determine_import_from_candidates(
candidates: ParsedJavascriptDependencyCandidate,
package_candidate_map: NodePackageCandidateMap,
tsconfig: TSConfig | None,
file_extensions: tuple[str, ...],
) -> Addresses:
paths = await path_globs_to_paths(
_add_extensions(
candidates.file_imports,
file_extensions,
)
)
local_owners = await Get(Owners, OwnersRequest(paths.files))

owning_targets = await Get(Targets, Addresses(local_owners))

addresses = Addresses(tgt.address for tgt in owning_targets)
if not local_owners:
non_path_string_bases = FrozenOrderedSet(
non_path_string.partition(os.path.sep)[0]
for non_path_string in candidates.package_imports
addresses = Addresses(())
# 1. handle relative imports
if candidates.file_imports:
Copy link
Contributor

Choose a reason for hiding this comment

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

This if-clause is ill-formed. It discards too much information.

paths = await path_globs_to_paths(
_add_extensions(
candidates.file_imports,
file_extensions,
)
)
addresses = Addresses(
package_candidate_map[pkg_name]
for pkg_name in non_path_string_bases
if pkg_name in package_candidate_map
local_owners = await Get(Owners, OwnersRequest(paths.files))
owning_targets = await Get(Targets, Addresses(local_owners))
addresses = Addresses(tgt.address for tgt in owning_targets)

# 2. handle package imports
elif candidates.package_imports:

# Try prepending the tsconfig root dir to the package import.
first_party_packge_imports = frozenset(
os.path.join(tsconfig.resolution_root_dir, pkg_import)
for pkg_import in candidates.package_imports
) if tsconfig and tsconfig.resolution_root_dir else frozenset()
paths = await path_globs_to_paths(
_add_extensions(
first_party_packge_imports,
file_extensions,
)
)
local_owners = await Get(Owners, OwnersRequest(paths.files))
Comment on lines +191 to +202
Copy link
Contributor

@tobni tobni Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This should be handled in the rust parser. If it isn't, it is a bug. See https://github.com/pantsbuild/pants/blob/6f8e16e0a2b78d9b3feeb860b38782b92abd1848/src/rust/engine/dep_inference/src/javascript/tests.rs#L473C4-L473C35

Copy link
Contributor

@tobni tobni Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This might also explain why I find the if-clause ill-formed. I think you might have misunderstood or been tricked by a bug in what the intention of ParsedJavascriptDependencyCandidate was. The parser can produce candidates on either form or both, but the separation you do here assumes it knows which case it found. It doesn't. That is why the method is named _determine_import_from_candidates. But like I said, the parser or my assumptions on how to read tsconfig might be bugged.


# 2.a. check for first-party package imports
if local_owners:
owning_targets = await Get(Targets, Addresses(local_owners))
addresses = Addresses(tgt.address for tgt in owning_targets)

# 2.b. check for third-party package imports
else:
# If package name begins with @, then keep the subpackage name
non_path_string_bases = FrozenOrderedSet(
f"{non_path_string.split('/')[0]}/{non_path_string.split('/')[1]}"
if non_path_string.startswith("@")
else non_path_string.partition(os.path.sep)[0]
for non_path_string in candidates.package_imports
)
Comment on lines +212 to +217
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

addresses = Addresses(
package_candidate_map[pkg_name]
for pkg_name in non_path_string_bases
if pkg_name in package_candidate_map
)
return addresses


Expand Down Expand Up @@ -243,7 +267,11 @@ async def infer_js_source_dependencies(
sources = await Get(
HydratedSources, HydrateSourcesRequest(source, for_sources_types=[JSRuntimeSourceField])
)
metadata = await _prepare_inference_metadata(request.field_set.address, source.file_path)
owning_pkg, maybe_config = await concurrently(
find_owning_package(OwningNodePackageRequest(request.field_set.address)),
find_parent_ts_config(ParentTSConfigRequest(source.file_path, "jsconfig.json"), **implicitly()),
)
metadata = await _prepare_inference_metadata(owning_pkg, maybe_config, request.field_set.address.spec_path)

import_strings, candidate_pkgs = await concurrently(
parse_javascript_deps(NativeDependenciesRequest(sources.snapshot.digest, metadata)),
Expand All @@ -258,6 +286,7 @@ async def infer_js_source_dependencies(
_determine_import_from_candidates(
candidates,
candidate_pkgs,
tsconfig=maybe_config.ts_config,
file_extensions=JS_FILE_EXTENSIONS + JSX_FILE_EXTENSIONS,
)
for string, candidates in import_strings.imports.items()
Expand Down
20 changes: 10 additions & 10 deletions src/python/pants/backend/typescript/dependency_inference/rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
)
from pants.backend.javascript.subsystems.nodejs_infer import NodeJSInfer
from pants.backend.javascript.target_types import JS_FILE_EXTENSIONS
from pants.backend.tsx.target_types import TSX_FILE_EXTENSIONS
from pants.backend.typescript import tsconfig
from pants.backend.typescript.target_types import (
TS_FILE_EXTENSIONS,
Expand Down Expand Up @@ -77,18 +78,13 @@ def _create_inference_metadata(
)


async def _prepare_inference_metadata(address: Address, file_path: str) -> InferenceMetadata:
owning_pkg, maybe_config = await concurrently(
find_owning_package(OwningNodePackageRequest(address)),
find_parent_ts_config(ParentTSConfigRequest(file_path, "tsconfig.json"), **implicitly()),
)

async def _prepare_inference_metadata(owning_pkg, maybe_config, spec_path) -> InferenceMetadata:
if not owning_pkg.target:
return InferenceMetadata.javascript(
(
os.path.dirname(maybe_config.ts_config.path)
if maybe_config.ts_config
else address.spec_path
else spec_path
),
{},
maybe_config.ts_config.resolution_root_dir if maybe_config.ts_config else None,
Expand All @@ -99,7 +95,6 @@ async def _prepare_inference_metadata(address: Address, file_path: str) -> Infer
maybe_config.ts_config,
)


@rule
async def infer_typescript_source_dependencies(
request: InferTypeScriptDependenciesRequest,
Expand All @@ -112,7 +107,11 @@ async def infer_typescript_source_dependencies(
sources = await Get(
HydratedSources, HydrateSourcesRequest(source, for_sources_types=[TypeScriptSourceField])
)
metadata = await _prepare_inference_metadata(request.field_set.address, source.file_path)
owning_pkg, maybe_config = await concurrently(
find_owning_package(OwningNodePackageRequest(request.field_set.address)),
find_parent_ts_config(ParentTSConfigRequest(source.file_path, "tsconfig.json"), **implicitly()),
)
metadata = await _prepare_inference_metadata(owning_pkg, maybe_config, request.field_set.address.spec_path)

import_strings, candidate_pkgs = await concurrently(
parse_javascript_deps(NativeDependenciesRequest(sources.snapshot.digest, metadata)),
Copy link
Contributor

@tobni tobni Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Word of warning that this parses javascript and JSX files, and will fail to parse as soon as it encounters the type keyword, which is a valid qualifier for typescript imports but not JS.

Expand All @@ -128,7 +127,8 @@ async def infer_typescript_source_dependencies(
_determine_import_from_candidates(
candidates,
candidate_pkgs,
file_extensions=TS_FILE_EXTENSIONS + JS_FILE_EXTENSIONS,
tsconfig=maybe_config.ts_config,
file_extensions=TS_FILE_EXTENSIONS + JS_FILE_EXTENSIONS + TSX_FILE_EXTENSIONS,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're supporting TSX here, should we also support JSX? I'm not sure, just pattern matching.

)
for string, candidates in import_strings.imports.items()
),
Expand Down