-
-
Notifications
You must be signed in to change notification settings - Fork 640
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
Add JSX support built on the JS backend #21206
Changes from all commits
3330c4a
3cb9365
de30fd3
6676f7c
4efaacb
9bab4e1
56caa50
6735f2d
d1db61c
55be737
74c8290
3b882c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,3 +68,4 @@ GTAGS | |
/.venv | ||
.tool-versions | ||
TAGS | ||
node_modules |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,11 +25,23 @@ | |
JS_TEST_FILE_EXTENSIONS = tuple(f"*.test{ext}" for ext in JS_FILE_EXTENSIONS) | ||
|
||
|
||
class JSDependenciesField(Dependencies): | ||
class JSRuntimeDependenciesField(Dependencies): | ||
"""Dependencies of a target that is javascript at runtime.""" | ||
|
||
|
||
class JSDependenciesField(JSRuntimeDependenciesField): | ||
pass | ||
|
||
|
||
class JSSourceField(SingleSourceField): | ||
class JSRuntimeSourceField(SingleSourceField): | ||
"""A source that is javascript at runtime.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feeding into the PR description. Thinking about this framing: AIUI, there's some tools that can run TypeScript natively (e.g. Bun; Node (experimentally) as of nodejs/node#53725), so a Does that play into the broader consideration, or maybe it's not a big-picture issue (e.g. maybe just this doc string is (very slightly) inaccurate)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose it is a point of semantics, but bun and node do not run typescript. They strip (node) or compile-down the (bun) types and run the resulting javascript. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think my key point is that there's tools for which the file run is the Brainstorming: I wonder if we could instead flip this around to be tool-focused using a union somehow (I'm not fully familiar with whether that's possible, though). E.g. register various source types as members of "can be passed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For specific tools were we can have a field set and union to target the tool, yes. Not sure how that works out for In other words, I think at some point we are required to "boil down" to a unique source field type to "end up" with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IDK either 🤷♂️ |
||
|
||
|
||
class JSTestRuntimeSourceField(SingleSourceField): | ||
"""A source that is runnable by javascript test-runners at runtime.""" | ||
|
||
|
||
class JSSourceField(JSRuntimeSourceField): | ||
expected_file_extensions = JS_FILE_EXTENSIONS | ||
|
||
|
||
|
@@ -86,7 +98,7 @@ class JSTestDependenciesField(JSDependenciesField): | |
pass | ||
|
||
|
||
class JSTestSourceField(JSSourceField): | ||
class JSTestSourceField(JSSourceField, JSTestRuntimeSourceField): | ||
expected_file_extensions = JS_FILE_EXTENSIONS | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# Copyright 2024 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
# NOTE: Sources restricted from the default for python_sources due to conflict with | ||
# - //:all-__init__.py-files | ||
# - //src/python/pants/backend/jsx/__init__.py:../../../../../all-__init__.py-files | ||
python_sources( | ||
sources=[ | ||
"target_types.py", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# Copyright 2024 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
python_sources() | ||
|
||
python_tests(name="tests") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
# Copyright 2024 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import annotations | ||
|
||
import dataclasses | ||
from dataclasses import dataclass | ||
from typing import Iterable | ||
|
||
from pants.backend.jsx.target_types import ( | ||
JSX_FILE_EXTENSIONS, | ||
JSXSourcesGeneratorTarget, | ||
JSXTestsGeneratorSourcesField, | ||
JSXTestsGeneratorTarget, | ||
) | ||
from pants.core.goals.tailor import ( | ||
AllOwnedSources, | ||
PutativeTarget, | ||
PutativeTargets, | ||
PutativeTargetsRequest, | ||
) | ||
from pants.core.util_rules.ownership import get_unowned_files_for_globs | ||
from pants.core.util_rules.source_files import classify_files_for_sources_and_tests | ||
from pants.engine.rules import Rule, collect_rules, rule | ||
from pants.engine.unions import UnionRule | ||
from pants.util.dirutil import group_by_dir | ||
from pants.util.logging import LogLevel | ||
|
||
|
||
@dataclass(frozen=True) | ||
class PutativeJSXTargetsRequest(PutativeTargetsRequest): | ||
pass | ||
|
||
|
||
@rule(level=LogLevel.DEBUG, desc="Determine candidate JSX targets to create") | ||
async def find_putative_jsx_targets( | ||
req: PutativeJSXTargetsRequest, all_owned_sources: AllOwnedSources | ||
) -> PutativeTargets: | ||
unowned_jsx_files = await get_unowned_files_for_globs( | ||
req, all_owned_sources, (f"*{ext}" for ext in JSX_FILE_EXTENSIONS) | ||
) | ||
classified_unowned_js_files = classify_files_for_sources_and_tests( | ||
paths=unowned_jsx_files, | ||
test_file_glob=JSXTestsGeneratorSourcesField.default, | ||
sources_generator=JSXSourcesGeneratorTarget, | ||
tests_generator=JSXTestsGeneratorTarget, | ||
) | ||
|
||
return PutativeTargets( | ||
PutativeTarget.for_target_type( | ||
tgt_type, path=dirname, name=name, triggering_sources=sorted(filenames) | ||
) | ||
for tgt_type, paths, name in (dataclasses.astuple(f) for f in classified_unowned_js_files) | ||
for dirname, filenames in group_by_dir(paths).items() | ||
) | ||
|
||
|
||
def rules() -> Iterable[Rule | UnionRule]: | ||
return ( | ||
*collect_rules(), | ||
UnionRule(PutativeTargetsRequest, PutativeJSXTargetsRequest), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the target discussion, for prettier specifically, it supports things that are very non-JS-shaped, e.g. HTML, GraphQL, Markdown, JSON, Yaml.
Two questions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think it is fun that you implied json is non-js shaped 😏 I get what you're saying though, you do not run a json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe, yeah.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I wrote the Prettier plugin a while back, and we did have the discussion about how it could be used target-less, but that (d)evolved into a much larger discussion about the needs for targets and target-less and so on and so forth.
There are a few tools where I think we just want to defer to whatever the tool can handle - and I don't know how we do that in the most pants-esque way