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.

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

backend: add support for TypeScript #20208

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AlexTereshenkov
Copy link
Member

This is an attempt to convert an internal plugin to a generic support of TypeScript for Pants.

  1. There are two new targets: typescript_sources and typescript_tests generators with respective single source targets, typescript_source and typescript_test.
  2. There is dependency inference enabled for them, being mostly driven by whatever inference the pure JS analyzer does. There's more work that needs to be done, see TODOs.
  3. There is support for the tailor goal with a gotcha; see TODOs.

Run these commands in the checkout of this branch:

$ pants --no-pantsd dependencies --transitive frontend/config/app.ts                               
08:01:26.19 [WARN] ('frontend/config/app.ts', NativeParsedJavascriptDependencies(file_imports=set(), package_imports={'services/browser', 'redux', '@sentry/react'}))
08:01:26.20 [WARN] ('frontend/services/browser.ts', NativeParsedJavascriptDependencies(file_imports=set(), package_imports={'utils/helpers', 'deployment'}))
08:01:26.21 [WARN] ('frontend/utils/helpers.ts', NativeParsedJavascriptDependencies(file_imports=set(), package_imports=set()))
08:01:26.21 [WARN] ('frontend/deployment/index.ts', NativeParsedJavascriptDependencies(file_imports=set(), package_imports=set()))
frontend/deployment/index.ts
frontend/services/browser.ts
frontend/utils/helpers.ts

@AlexTereshenkov AlexTereshenkov changed the title Backend/add typescript targets backend: add support for TypeScript Nov 19, 2023
@chrisjrn
Copy link
Contributor

@AlexTereshenkov I see the /frontend directory is probably intended to be temporary, but /testprojects/src is available, and there's no reason why that directory structure can't live there.

@AlexTereshenkov
Copy link
Member Author

@AlexTereshenkov I see the /frontend directory is probably intended to be temporary, but /testprojects/src is available, and there's no reason why that directory structure can't live there.

Yes, certainly this is just to ease testing things out for everyone :) not intending to merge it at all, tbh. We'll have tests later.

@chrisjrn
Copy link
Contributor

@AlexTereshenkov I know you're not intending to merge it, but there are other things like this over in /testprojects/src, so why not keep it around?

Comment on lines +206 to +209
for package in import_strings.package_imports:
if package.startswith("@"):
# it's a 3rd-party package?
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a correct assumption.

# having `import { foo } from 'bar';` means there may be
# `src/bar/index.ts` if `bar` is a first-party code package
for ext in TS_FILE_EXTENSIONS:
# TODO: the import statements are not providing absolute paths; we have to figure out somehow what address
Copy link
Contributor

Choose a reason for hiding this comment

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

We need special handling of index.ts/js files and what names they point to. We also need to treat the ambiguity problem of naming local top-level folders the same as third party packages the same as TS (I dont recall it offhand but I believe it prefers local over 3rd party?).

I believe tsconfig root directories and index.ts modules have to be slurped into the inference impl to behave correctly.

return InferredTypeScriptDependencies(addresses)


@rule("Get dependencies of typescript_test targets.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely all of these can be just the one rule?

addresses = []

for path in import_strings.file_imports:
# check if an import is a directory (that should contain `index.ts` or `index.tsx`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be handled similarily to how __init__.py files are treated I believe, and then fed into the rust parser as a pattern via the metadata.

Copy link
Contributor

@tobni tobni Nov 20, 2023

Choose a reason for hiding this comment

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

Meaning a line such as

import * from "myMod"

should be inferred as rootDir/myMod/index.ts
because we ahead of time know all index.ts file locations, and have constructed patterns like:

    {"myMod": ["./myMod/index.ts"]}

)
from pants.util.strutil import help_text

TS_FILE_EXTENSIONS = (".ts", ".tsx")
Copy link
Contributor

@tobni tobni Nov 20, 2023

Choose a reason for hiding this comment

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

I doubt we're ready to deal with tsx at this stage? It invites a lot of questions of how we (wont/dont) handle the transpilation of these sources. I understand this is to enable the test goal, in which the runner transpiles stuff on the fly, but I'm fairly sure running the compiler is what comes next? And in that case we'd want special targets for .tsx that it doesnt touch, because it cant parse them.

@tobni
Copy link
Contributor

tobni commented Nov 20, 2023

Just want to add that I think its really nice to see things move in this area!

@tobni
Copy link
Contributor

tobni commented Nov 20, 2023

I'll also add that ts files can depend on js, and I'm not sure whether the inference impl here deals with that?

AlexTereshenkov added a commit that referenced this pull request Dec 1, 2023
Work towards #20208

Adding targets for TypeScript sources and tests.
AlexTereshenkov added a commit that referenced this pull request Dec 6, 2023
Work towards #20208.

Add tailor goal to create `typescript_sources` and `typescript_tests`
targets.

```
$ find testprojects/src/ts -name "BUILD" -type f -delete
$ pants tailor testprojects/src/ts::
Created testprojects/src/ts/frontend/config/BUILD:
  - Add typescript_sources target config
Created testprojects/src/ts/frontend/deployment/BUILD:
  - Add typescript_sources target deployment
Created testprojects/src/ts/frontend/services/BUILD:
  - Add typescript_sources target services
  - Add typescript_tests target tests
Created testprojects/src/ts/frontend/utils/BUILD:
  - Add typescript_sources target utils
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants