Add ts-node integration as sucrase/ts-node-plugin #729
Conversation
Fixes #726 This makes it easy to use Sucrase with all of the nice benefits that ts-node provides, such as an ESM loader, tsconfig discovery, and a REPL. The implementation is based off of these docs: https://typestrong.org/ts-node/docs/transpilers/ and the built-in SWC integration: https://github.com/TypeStrong/ts-node/blob/main/src/transpilers/swc.ts Some implementation notes: * Currently, the plugin is written as a CJS module and written in JS rather than TS. This seems like the most reasonable initial approach since currently all build artifacts go to a `dist` directory, and it's important to not expose that fact in the usage. The future plan is to add `exports` config to package.json in a semver-major release, which should clean this up. * The plugin is included as part of the core `sucrase` package rather than as an integration to be installed separately. This should make the usage and version management a little easier, and feels reasonable because the integration is small and has zero dependencies. * I rewrote the usage section of the README to put installation instructions at the top and to suggest ts-node as the recommended way to use Sucrase with Node.
Codecov Report
@@ Coverage Diff @@
## main #729 +/- ##
=======================================
Coverage 87.51% 87.51%
=======================================
Files 55 55
Lines 5887 5887
Branches 1394 1394
=======================================
Hits 5152 5152
Misses 466 466
Partials 269 269 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Benchmark resultsBefore this PR: 338.7 thousand lines per second Measured change: 0.01% faster (0.56% slower to 3.24% faster) |
| /** | ||
| * ts-node transpiler plugin | ||
| */ | ||
| function create(createOptions) { |
There was a problem hiding this comment.
@cspotcode Here's what I have so far. Note that plenty of tsconfig options aren't supported by Sucrase (e.g. target ES level), so my plan is to just silently go with Sucrase defaults.
In terms of high-level plugin interface feedback, the two-level create/transpile approach doesn't seem particularly important for my use case, but I can certainly imagine it being useful for other cases. Currently I'm just doing all work in transpile, though I suppose I could front-load some of it.
There was a problem hiding this comment.
nodeModuleEmitKind is marked @internal in ts-node's API, so I should update to make this a public part of the API. Will serve as documentation and as a reminder that we cannot make breaking changes. I have called this out in TypeStrong/ts-node#1851
ts-node-plugin/index.js
Outdated
| filePath: fileName, | ||
| preserveDynamicImport: nodeModuleEmitKind === "nodecjs", | ||
| injectCreateRequireForImportRequire: nodeModuleEmitKind === "nodeesm", | ||
| disableESTransforms: true, |
There was a problem hiding this comment.
The ES transforms in Sucrase are pretty irrelevant these days when targeting node, so I just disable them (rather than try to read target from tsconfig), and I'm planning on removing them in an upcoming breaking change.
| if (module === ModuleKindCommonJS || nodeModuleEmitKind === "nodecjs") { | ||
| transforms.push("imports"); | ||
| } | ||
| if (fileName.endsWith(".tsx") || fileName.endsWith(".jsx")) { |
There was a problem hiding this comment.
@cspotcode can you confirm that I'm understanding this right?
- The right way to detect if the JSX transform should be enabled is by looking at the file extension.
- The right way to detect if the file is TypeScript or JavaScript is to look at the file extension. (Unclear if there's much value in disabling the TS transform for JS files, so maybe I'll just leave it enabled for the sake of simplicity, which the swc integration seems to do.)
- ts-node and TS do not allow JSX syntax inside .js files. (I believe for non-TS projects it's fairly common to do that.)
There was a problem hiding this comment.
JSX detection:
looks like you are correct: https://github.com/TypeStrong/ts-node/blob/97f9afd046b66a0fe05a7d76e7a32f94b872016f/src/transpilers/swc.ts#L65-L68
However, this might be a case where ts-node should take responsibility and pass this as a flag to transpilers. transpileOptions.isJsx or something like that.
TS vs JS detection:
Embarrassingly, the swc integration doesn't attempt this at the moment, it assumes everything is TS. (issue: TypeStrong/ts-node#1801) Again, perhaps this is a case where ts-node should pass as an isTs flag to make things clearer.
Re JSX syntax inside JS files: Pretty sure you are correct, though at a higher level, my philosophy is to support whatever the TS language service supports. So if projects are using TS's editor tooling, and it is happy to do tooltips, tab complete, etc with JSX inside JS files, perhaps using specific tsconfig flags, then I want to support that.
| @@ -0,0 +1,38 @@ | |||
| const {transform} = require("../dist"); | |||
|
|
|||
| const ModuleKindCommonJS = 1; | |||
There was a problem hiding this comment.
@cspotcode Having to reference TypeScript's internal enum values is probably the most awkward part of the plugin interface, though it seems very understandable in giving the most flexibility in mapping TS options to transpiler options. I'm assuming it's extremely unlikely that TypeScript would change these values?
There was a problem hiding this comment.
That's my understanding, yeah. FWIW the swc plugin already duplicates some of those enums. Not sure if it's helpful for ts-node to expose these lookups as part of the plugin API to make things easier on your end?
There was a problem hiding this comment.
Yep, I saw what the swc plugin was doing and copied that approach. Exposing named values (rather than just numbers) could be nice, though I could imagine that being a pain to maintain as TS adds more options and more enum values, and it sounds like I'd only be able to use whatever is defined in the minimum ts-node version that I support. So my impression is that the current approach makes the most sense.
| transforms.push("imports"); | ||
| } | ||
| if (fileName.endsWith(".tsx") || fileName.endsWith(".jsx")) { | ||
| transforms.push("jsx"); |
There was a problem hiding this comment.
Currently, Sucrase only supports the old JSX transform; see #585. If the tsconfig is configured for the new transform, an open question is whether to crash with a message that it's unsupported, or if it's better to just transpile with the old transform anyway. Anyone using the new transform likely has at least one file without import React from "react"; (or is using jsxImportSource), so it's probably better to just crash here to give a more helpful error message.
There was a problem hiding this comment.
A related thing I want to get right: ensuring plugins have a good way to raise diagnostics and / or warnings. ts-node doesn't do many warnings today, but I want to add a good mechanism for it. So that ts-node and/or transpiler plugins can warn users about potential config issues, version incompatibilities, stuff like that. And then we can support config flags to suppress warnings either completely, or suppress warnings by diagnostic code.
What this would mean for sucrase: you have the option of returning either errors or warnings from either create() or transpile() and ts-node will handle either throwing or logging, or suppressing based on tsconfig. I'll try to jot down ideas in TypeStrong/ts-node#1851
|
I left responses to everything above. One other thing not mentioned: As a breaking change, I'd like to be more automatic about overriding compiler options. Users can already do this explicitly via E.g. for a frontend project where tsconfig is setup for browsers, we can override with Anyway I mention this cuz it seems to align with what you're doing as far as automatically defaulting to whatever's going to work in node even if it conflicts with tsconfig |
* JSX config * allowJS * esModuleInterop
package.json
Outdated
| "peerDependencies": { | ||
| "ts-node": ">=10.9.1" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "ts-node": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
@cspotcode I've been trying to wrap my head around the right way to think about peer dependencies and compatibility expectations here and wanted to get your thoughts and any recommendations.
I based these package.json lines off of the peer dependency you have on SWC; I hadn't previously seen peerDependenciesMeta and optional, but definitely seems like the right approach for this sort of integration.
The >= here is certainly convenient for me because Sucrase won't need to be updated at every ts-node semver-major release, but the worry, of course, is that ts-node breaking changes will break the plugin API. I suppose for the Sucrase case specifically, you'll presumably be careful to not break sucrase: true, so we can coordinate whatever migration path makes sense as necessary. But thinking more generally, I could see three ways to think about it:
- The
>=is correct because ts-node can commit to not breaking old plugins, not even in semver-major releases. - ts-node may have breaking changes to the plugin interface, but the
>=is fine because it still provides the guarantee to users that their tools will only break while doing a semver-major upgrade to some tool (ts-nodeorsucrase). The main goal is to avoid a surprise break while doing a minor upgrade or regenerating a lockfile, and ifts-nodebreaks the plugin interface, users might need to upgradesucraseor wait for a new version. - The
>=is incorrect, and it would be better to list the major versions that I know are compatible and try to keep them updated in a timely way.
Sounds like maybe the second bullet is the most reasonable way to think about it.
If I'm following the advice at semver/semver#502 (comment) , any increase to the minimum ts-node version here is semver-major for Sucrase. It certainly makes sense; re-generating your lockfile (or doing a minor upgrade to one tool) should never break your tools. Some consequences to think about in this case:
- If I release right now and then you later add new convenience features like
isTSorisJSXin a new version, I won't be able to rely on them until the next Sucrase semver-major release, since I need to still support ts-node 10.9.1. Other types of changes like optional diagnostics in the response would be fine for me to add, as long as ts-node doesn't break on extraneous fields now (and I see that it doesn't). - If ts-node releases a breaking change without providing a path for me to maintain support for old and new ts-node versions in the same plugin, that puts me in a hard spot because it would mean that the only way for me to fix compatibility is with a semver-major upgrade (which I'd generally prefer to avoid). One ad hoc way to avoid this problem is to pick a different function name, e.g. the old API is
createand the new one iscreateTranspilerand I include both for compatibility for a while and then eventually drop the oldcreatein a breaking change. From Sucrase integration TypeStrong/ts-node#1851, sounds like you also had some ideas around API version to coordinate this.
Another approach to all of this is to have another package @sucrase/ts-node-plugin that people install in addition to the other two. That would make major version bumps less of a big deal since it's just the integration with the semver-major change, but I think including it in the core sucrase package certainly seems nicest from a DX perspective.
Maybe I'm overthinking these things and there won't be a need for significant breaking changes, just thinking through how it would all work. Like I mentioned, I'm sure we can coordinate since you wouldn't want sucrase: true to break anyway.
My impression from the conversation so far is that breaking changes won't actually be necessary and the existing interface makes additive changes pretty easy, but would be good to know if you have a significant breaking change in mind. To be clear, I'd be fine shipping this as-is without the API interface improvements and then picking them up whenever I do a semver-major upgrade in the future, or I'd also be fine waiting for improvements or breaking changes to the interface if that makes the most sense.
There was a problem hiding this comment.
After some more thought and testing, I'm now thinking that I won't add even an optional peer dependency, since it could break npm users where a project has sucrase and ts-node installed independently without plans to use the plugin. I can always add docs as needed or possibly other approaches to ensure compatibility, but for now will keep it simple.
ts-node-plugin/index.js
Outdated
| throw new Error( | ||
| 'The JSX modes "react-jsx" and "react-jsxdev" are not yet ' + | ||
| 'supported by Sucrase. Consider using "react" as a workaround.', | ||
| ); |
There was a problem hiding this comment.
@cspotcode this is the one error that I have in mind right now, and my impression is that it's most helpful to just fail fast because it means a configuration that's unlikely to work with Sucrase. The Sucrase transpiler itself doesn't give nice diagnostics, it just throws SyntaxError (or similar) if it hits a problem, though I could wrap in a try/catch and return some type of failure if you think that's better.
From some testing, npx is a little slower than invoking ts-node directly, so I don't necessarily want to recommend it as the thing to always use.
|
@cspotcode Thanks for all the feedback earlier. I'm planning to release this as-is, can certainly iterate on interface details in the future. |
Fixes #726
This makes it easy to use Sucrase with all of the nice benefits that ts-node
provides, such as an ESM loader, tsconfig discovery, and a REPL.
The implementation is based off of these docs:
https://typestrong.org/ts-node/docs/transpilers/
and the built-in SWC integration:
https://github.com/TypeStrong/ts-node/blob/main/src/transpilers/swc.ts
Some implementation notes:
TS. This it the easiest approach with the current build system, though it may
be reasonable to extend the build system to compile a
ts-node-plugin-srcdirectory to
ts-node-pluginor something like that. The future plan is to addan
exportsline topackage.jsonin a semver-major release, which will makethis a bit easier to manage.
sucrasepackage rather than as anintegration to be installed separately. This should make the usage and version
management a little easier, and feels reasonable because the integration is
small and has zero dependencies.
the top and to suggest ts-node as the recommended way to use Sucrase with Node.
integration-testdirectory with various cases that this plugin shouldhandle, and it may be useful for future node/tooling integration tests as well.
This is a draft PR. Remaining work to do:
allowJSand transpiling JS Files (with tests)esModuleInterop