feat: add emitIsolatedDts support #270
Conversation
jbedard
force-pushed
the
dts_outs
branch
3 times, most recently
from
002ac69
to
84b06f8
Compare
swc/defs.bzl
Outdated
| js_outs = _swc_lib.calculate_js_outs(srcs, out_dir, root_dir) | ||
| map_outs = _swc_lib.calculate_map_outs(srcs, source_maps, out_dir, root_dir) | ||
| if kwargs.get("experimental_emit_isolated_dts", False): | ||
| dts_outs = _swc_lib.calculate_dts_outs(srcs, out_dir, root_dir) |
There was a problem hiding this comment.
can it produce d.ts.map files too, like tsc does?
There was a problem hiding this comment.
From what I can find it seems like swc can do one or the other right now, not both. If you pass --out-file more then once it's an error, if you don't pass it at all it outputs only the .js to stdout instead...
There was a problem hiding this comment.
Wait, I read that wrong... I meant it can't produce .js and .d.ts, I'll look into .d.ts.map more
swc/private/swc.bzl
Outdated
| @@ -66,6 +66,10 @@ https://docs.aspect.build/rulesets/aspect_rules_js/docs/js_library#data for more | |||
| "root_dir": attr.string( | |||
| doc = "a subdirectory under the input package which should be consider the root directory of all the input files", | |||
| ), | |||
| "experimental_emit_isolated_dts": attr.bool( | |||
There was a problem hiding this comment.
Did you consider whether or not this ought to have the experimental_ prefix? When swc changes their flag, will we also change our attribute name? That will be breaking, right?
There was a problem hiding this comment.
I want to be able to make breaking changes to this still so I figured putting experimental_ right in the API is a good way to do that for now. Then it will be breaking when we rename to make it stable and we'll have to keep it non-breaking from that point forward.
There was a problem hiding this comment.
We've never had an attribute prefixed with experimental_ before, and never wrote that it's not public API. I guess it's a fine convention, just surprised that we invent a new one so many years in.
Perhaps it should be exposed as a separate rule instead? Doesn't need to mean copy-paste of code, probably 95% reused in a lib.
There was a problem hiding this comment.
I could also put EXPERIMENTAL in the docs I guess, but I don't want this to be considered a stable public API at this point. I guess I was partially just following the jsc.experimental.emitIsolatedDts swc property 🤷
swc/private/swc.bzl
Outdated
| @@ -66,6 +66,10 @@ https://docs.aspect.build/rulesets/aspect_rules_js/docs/js_library#data for more | |||
| "root_dir": attr.string( | |||
| doc = "a subdirectory under the input package which should be consider the root directory of all the input files", | |||
| ), | |||
| "experimental_emit_isolated_dts": attr.bool( | |||
| doc = "Emit .d.ts files for TypeScript sources", | |||
There was a problem hiding this comment.
add "instead of .js outputs" ?
There was a problem hiding this comment.
Also please link to SWC docs or discussion threads about the feature to save the reader some effort in researching this new capability
There was a problem hiding this comment.
There really is no documentation at the moment other then the commit in the changelog: https://github.com/swc-project/swc/blob/main/CHANGELOG.md#164---2024-06-22
| @@ -248,6 +278,15 @@ def _swc_impl(ctx): | |||
| inputs.extend(ctx.files.plugins) | |||
| args.add_all(plugin_args) | |||
|
|
|||
| if ctx.attr.experimental_emit_isolated_dts: | |||
| args.add_all(["--config-json", json.encode({ | |||
There was a problem hiding this comment.
I assume you tested that --config-file foo --config-json '{"some": "object"}' has the desired behavior of overriding the file?
What happens if the user also gives a --config-json in the args they provide?
There was a problem hiding this comment.
Also would be good to comment if there's an ordering constraint between these two flags
There was a problem hiding this comment.
These are a few things that I think need more testing, both here and our other --config-json cases (see plugins).
Note that we could also go the route where if experimental_emit_isolated_dts = True then we assume you set this emitIsolatedDts flag yourself, then maybe add some "validation" in the future like tsc does.
IMO if --config-json merges+overwrites the existing config then I prefer this instead of requiring validation, but both have trade-offs.
|
I think we're blocked for now on swc-project/swc#9512 if I'm understanding correctly. The way we invoke the cli using |
Add experimental support for the
jsc.experimental.emitIsolatedDtsfeature to output.d.tsinstead of.js.Changes are visible to end-users: no
Test plan