Add package support to typescript/javascript dependency inference #21556

jasondamour · 2024-10-18T23:11:02Z

This adds better dependency inference to typescript/javascript.

Adds support for imports relative to tsconfig root
a. i.e. utils/stringUtils -> frontend/src/utils/stringUtils.ts
Adds support for namespaced package imports
a. @apollo/client -> :package#__apollo/client
Adds support for importing .tsx from .ts files
a. This is a weird one, I had to add this to support my usecase but I'm not sure the "right" way to handle js/jsx/ts/tsx cross-imports.

I'm not how much of the javascript "spec" I covered

Sample file

# example.ts
import 'axios'
import '@apollo/client'
import 'utils/stringUtils'
import 'utils/tableUtils'
import '../utils/urlUtils'

Produces

PANTS_SOURCE=../../OSS/pants pants --no-pantsd dependecies example.ts
frontend/src/utils/stringUtils.ts:../../typescript
frontend/src/utils/tableUtils.tsx:../../tsx
frontend/src/utils/urlUtils.ts:../../typescript
frontend:package#__apollo/client
frontend:package#axios

huonw

Thanks for this. Is it possible to add some tests to the adjacent rules_test.py files?

huonw · 2024-10-24T05:31:53Z

src/python/pants/backend/javascript/dependency_inference/rules.py

-        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:


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

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

huonw · 2024-10-24T05:34:05Z

src/python/pants/backend/typescript/dependency_inference/rules.py

@@ -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,


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

tobni · 2024-11-03T20:51:37Z

src/python/pants/backend/javascript/dependency_inference/rules.py

+        # 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))


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

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.

tobni · 2024-11-03T21:01:06Z

src/python/pants/backend/javascript/dependency_inference/rules.py

-            for non_path_string in candidates.package_imports
+    addresses = Addresses(())
+    # 1. handle relative imports
+    if candidates.file_imports:


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

tobni · 2024-11-03T21:03:58Z

src/python/pants/backend/typescript/dependency_inference/rules.py

+        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)),


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.

tobni · 2024-11-03T21:27:05Z

You need to provide the tsconfig or full repro to be able to validate your example.

"Adds support for imports relative to tsconfig root", this is not a default of the ts resolution spec, but might be for your tooling. If it is, I'm going to encourage you set your tsconfig to match what the tooling is implicitly doing, instead of merging "1a" of this change.

You generally need to supply "paths" entries in the tsconfig that maps to the "any" match (*) to get behaviour like that, from what I understand. See https://www.typescriptlang.org/tsconfig/#paths and https://www.typescriptlang.org/docs/handbook/modules/reference.html#paths

If tests prove "2a" I think that would be a good bug fix.

3a should adhere to https://www.typescriptlang.org/docs/handbook/modules/reference.html#file-extension-substitution.

I just want to reiterate that the parser is a javascript parser, not a typescript parser. It is not up to the task of parsing any typescript specifics, so please tread lightly.

tobni · 2024-11-03T22:05:51Z

src/python/pants/backend/javascript/dependency_inference/rules.py

+            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
+            )


Good catch!

Add package support to typescript/javascript dependency inference

52fb0e3

jasondamour marked this pull request as ready for review October 18, 2024 23:11

benjyw requested a review from tobni October 21, 2024 13:17

huonw requested a review from AlexTereshenkov October 24, 2024 05:29

huonw added the category:new feature label Oct 24, 2024

huonw reviewed Oct 24, 2024

View reviewed changes

tobni reviewed Nov 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add package support to typescript/javascript dependency inference #21556

Add package support to typescript/javascript dependency inference #21556

jasondamour commented Oct 18, 2024 •

edited

Loading

huonw left a comment

huonw Oct 24, 2024

jasondamour Oct 24, 2024

huonw Oct 24, 2024

tobni Nov 3, 2024 •

edited

Loading

tobni Nov 3, 2024 •

edited

Loading

tobni Nov 3, 2024

tobni Nov 3, 2024 •

edited

Loading

tobni commented Nov 3, 2024 •

edited

Loading

tobni Nov 3, 2024

Add package support to typescript/javascript dependency inference #21556

Are you sure you want to change the base?

Add package support to typescript/javascript dependency inference #21556

Conversation

jasondamour commented Oct 18, 2024 • edited Loading

Sample file

Produces

huonw left a comment

Choose a reason for hiding this comment

huonw Oct 24, 2024

Choose a reason for hiding this comment

jasondamour Oct 24, 2024

Choose a reason for hiding this comment

huonw Oct 24, 2024

Choose a reason for hiding this comment

tobni Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

tobni Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

tobni Nov 3, 2024

Choose a reason for hiding this comment

tobni Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

tobni commented Nov 3, 2024 • edited Loading

tobni Nov 3, 2024

Choose a reason for hiding this comment

jasondamour commented Oct 18, 2024 •

edited

Loading

tobni Nov 3, 2024 •

edited

Loading

tobni Nov 3, 2024 •

edited

Loading

tobni Nov 3, 2024 •

edited

Loading

tobni commented Nov 3, 2024 •

edited

Loading