Skip to content

Conversation

alexeagle
Copy link
Member

@alexeagle alexeagle commented Jan 7, 2023

I think we might be able to roll this out in a 1.x release. If we get good compatibility, it should only be breaking in that WORKSPACE needs swc dep/toolchain added. Bzlmod users (all zero of them) would be unaffected.

@gregmagolan
Copy link
Member

Users will also need to configure their swcrc settings which for many users they have no idea what that is. Perhaps auto-generate sane values from tsconfig?

@bhavitsharma
Copy link

Does swc work well with node packages? AFAIK, it's only for browser targets.

@gregmagolan
Copy link
Member

Does swc work well with node packages? AFAIK, it's only for browser targets.

The compiled output from swc can be used both in node and in the browser.

@alexeagle
Copy link
Member Author

Note, this is more of an RFC - it will be a serious project and needs an owner, probably not me.

@alexeagle
Copy link
Member Author

There's already some decent-looking code here to generate the swcrc from a tsconfig
https://github.com/Songkeys/tsconfig-to-swcconfig/blob/main/src/index.ts#L18
I bet it handles 95% of the use cases so it would be good enough to vendor/dep on that in our toolchain to get swc to produce approx. the same JS that tsc was writing, with no user changes required.
I think there's enough risk with it that it's breaking and should go in a 2.0, but I don't think that slows us down.

@matthewjh
Copy link

matthewjh commented Jan 15, 2023

Some caveats of using SWC that warrant consideration if this to become a default:

  1. Where SWC can't figure out based on local information whether an import of a symbol should be a "type import" and not emitted, users need to use type-only imports (and exports). Two example situations where this arises most prominently are emitDecoratorMetadata (which introduces ambiguity as symbols in type positions can be "promoted" to value symbols) and files without any usage information like:
export {A, B} from "./index";
// is A going to exist at runtime?
// is B?

This is "breaking" because if SWC emits an import for a symbol that doesn't exist in the corresponding file, that will break downstream in bundling or loading the module via Node.

  1. Consuming const enums from NPM packages doesn't really work in SWC. The const enum doesn't exist at runtime, and SWC doesn't have the information to inline the value as TSC does. Some libraries export const enums.

As a side note, I wonder what % of ts_projects are going to be behind linked npm_packages. In this case, the most likely outcome is that both TSC and SWC run side-by-side, because the distinction and ability to select types vs sources is lost in the TreeArtifact. For this reason, we have separate configurations: one that uses TSC and produces .js and d.ts (slower but needed for typecheck and eslint), and another that uses SWC and only includes .js in npm_packages (faster for devving).

@gregmagolan
Copy link
Member

Perhaps the recommended pattern for ts_projects behind linked npm_packages should be to creates a runtime foo and a typecheck @types/foo to avoid the runtime bottleneck dependency on tsc

@matthewjh
Copy link

Perhaps the recommended pattern for ts_projects behind linked npm_packages should be to creates a runtime foo and a typecheck @types/foo to avoid the runtime bottleneck dependency on tsc

I considered that, but it seemed too much boilerplate as then all the deps need to be duplicated to list the @types packages. Maybe that could be automated, though, and x and @types/x considered a "pair" of sorts.

@gregmagolan
Copy link
Member

I considered that, but it seemed too much boilerplate as then all the deps need to be duplicated to list the @types packages. Maybe that could be automated, though, and x and @types/x considered a "pair" of sorts.

I could see an npm_package_pair rule of sorts that handles the boilerplate

@alexeagle
Copy link
Member Author

I think our next step here can be to make rules_ts depend on the 1.0 of rules_swc (hopefully this week) and then improve the ease of choosing swc as the transpiler, maybe by making it possible to use transpiler = "swc" (just a string value), and providing some affordance for the tsconfig->.swcrc translation. I think the @types/foo side-channel packaging above is also wise (we've talked before about understanding [foo, @types/foo] pairs so they don't have to be repeated in deps)

Then document this as a preferred path and respond to issues users are running into. Then we'll be able to evaluate whether we've made it convenient enough to become the default sometime.

This plan doesn't relieve pressure on making the persistent workers more hermetic and correct though.

alexeagle added a commit to aspect-build/rules_swc that referenced this pull request Jan 22, 2023
Useful as part of aspect-build/rules_ts#289
to make a migration path easier for those using tsc for transpiling
alexeagle added a commit to aspect-build/rules_swc that referenced this pull request Jan 22, 2023
Useful as part of aspect-build/rules_ts#289
to make a migration path easier for those using tsc for transpiling
alexeagle added a commit to aspect-build/rules_swc that referenced this pull request Jan 22, 2023
Useful as part of aspect-build/rules_ts#289
to make a migration path easier for those using tsc for transpiling
alexeagle added a commit to aspect-build/rules_swc that referenced this pull request Jan 23, 2023
Useful as part of aspect-build/rules_ts#289
to make a migration path easier for those using tsc for transpiling
@gregmagolan gregmagolan added enhancement New feature or request breaking Requires a semver-major release due to breaking public API changes wip labels Feb 6, 2023
@alexeagle alexeagle added this to the 2.0 milestone Apr 3, 2023
@alexeagle
Copy link
Member Author

picking this back up with a fresh PR, new plan is just to force users to pick a default transpiler like we did for skipLibCheck

@alexeagle alexeagle closed this Aug 7, 2023
@alexeagle alexeagle deleted the swc_default branch May 22, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Requires a semver-major release due to breaking public API changes enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants