Skip to content

Conversation

@marco-ippolito
Copy link
Member

@marco-ippolitomarco-ippolito commented Aug 9, 2024

With the new flag --experimental-transform-types it is possible to enable the transformation of TypeScript-only syntax into JavaScript code.
This feature allows Node.js to support TypeScript syntax such as Enum and namespace.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/typescript

@marco-ippolitomarco-ippolito changed the title module: add --experimental-enable-transformation for strip-types[WIP] module: add --experimental-enable-transformation for strip-typesAug 9, 2024
@nodejs-github-botnodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 9, 2024
@marco-ippolitomarco-ippolito added wip Issues and PRs that are still a work in progress. strip-types Issues or PRs related to strip-types support labels Aug 9, 2024
@marco-ippolitomarco-ippolitoforce-pushed the feat/enable-transformations branch from 29040a6 to c5da92eCompareAugust 9, 2024 11:20
@marco-ippolitomarco-ippolito marked this pull request as ready for review August 9, 2024 11:20
@marco-ippolitomarco-ippolito changed the title [WIP] module: add --experimental-enable-transformation for strip-typesmodule: add --experimental-enable-transformation for strip-typesAug 9, 2024
@marco-ippolitomarco-ippolito removed the wip Issues and PRs that are still a work in progress. label Aug 9, 2024
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@marco-ippolitomarco-ippolito added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 9, 2024
@marco-ippolitomarco-ippolitoforce-pushed the feat/enable-transformations branch 2 times, most recently from de62233 to 7343717CompareAugust 9, 2024 12:15
Copy link
Member

@legendecaslegendecas left a comment

Choose a reason for hiding this comment

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

Can we also add a test case that --experimental-enable-transformation doesn't eliminate unused imports, like this example?

@marco-ippolitomarco-ippolitoforce-pushed the feat/enable-transformations branch from 7343717 to b67230fCompareAugust 9, 2024 12:58
@marco-ippolitomarco-ippolitoforce-pushed the feat/enable-transformations branch from b67230f to e0ac82eCompareAugust 9, 2024 13:27
@marco-ippolitomarco-ippolitoforce-pushed the feat/enable-transformations branch from e0ac82e to 3578d86CompareAugust 9, 2024 13:35
@marco-ippolitomarco-ippolito changed the title module: add --experimental-enable-transformation for strip-typesmodule: add --experimental-enable--type-transform for strip-typesAug 9, 2024
@marco-ippolitomarco-ippolito changed the title module: add --experimental-enable--type-transform for strip-typesmodule: add --experimental-enable-type-transform for strip-typesAug 9, 2024
@marco-ippolitomarco-ippolitoforce-pushed the feat/enable-transformations branch from 3578d86 to 6b155bdCompareAugust 9, 2024 13:37
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

Actually, on closer look I have a change request: I think --experimental-enable-type-transform should imply --enable-source-maps.

(I'm +1 on the feature)

@marco-ippolito
Copy link
MemberAuthor

Actually, on closer look I have a change request: I think --experimental-enable-type-transform should imply --enable-source-maps.

(I'm +1 on the feature)

Probably makes more sense, otherwise location will be always wrong

@marco-ippolitomarco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 12, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
@nodejs-github-botnodejs-github-bot merged commit 0301309 into nodejs:mainAug 12, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 0301309

@statianzo
Copy link

Has there been discussion around writing out the result of typescript transformation?

It would allow package creators to publish TS -> JS consistent with the transformations node is doing internally. Given transformations don't apply to node_modules, packages authored using TS will require additional tooling to apply before the ability to publish. If publishers opt for tsc it wouldn't exactly match the calling the .ts from node directly.

Happy to post this comment elsewhere if more suitable.

@marco-ippolito
Copy link
MemberAuthor

marco-ippolito commented Aug 12, 2024

Has there been discussion around writing out the result of typescript transformation?

It would allow package creators to publish TS -> JS consistent with the transformations node is doing internally. Given transformations don't apply to node_modules, packages authored using TS will require additional tooling to apply before the ability to publish. If publishers opt for tsc it wouldn't exactly match the calling the .ts from node directly.

Happy to post this comment elsewhere if more suitable.

We have a repository for this kind of discussion nodejs/typescript

@Bnaya
Copy link

Sorry if i'm later to the party :)
Does it supports only the features that are supported by typescript's isolatedModules, or also the cross-file features?

@jakebailey
Copy link
Member

No, this is just isolatedModules. Node.js is not shipping a type checker that can perform that kind of analysis.

targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54283 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54283 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
@targostargos added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Aug 14, 2024
@targos
Copy link
Member

I optimistically pushed it to v22.x-staging but tests failed, so I took it out: https://github.com/nodejs/node/actions/runs/10382663326/job/28746123871

@marco-ippolito
Copy link
MemberAuthor

I optimistically pushed it to v22.x-staging but tests failed, so I took it out: https://github.com/nodejs/node/actions/runs/10382663326/job/28746123871

It requires backport I think, to imply the experimental-detect-module

marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Aug 14, 2024
PR-URL: nodejs#54283 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
@marco-ippolitomarco-ippolito removed the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Aug 19, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 20, 2024
PR-URL: #54283 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 20, 2024
Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452
@RafaelGSSRafaelGSS mentioned this pull request Aug 20, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
PR-URL: #54283 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 21, 2024
Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452
RafaelGSS added a commit that referenced this pull request Aug 22, 2024
Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452
@targostargos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.dont-land-on-v20.xPRs that should not land on the v20.x-staging branch and should not be released in v20.x.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.notable-changePRs with changes that should be highlighted in changelogs.semver-minorPRs that contain new features and should be released in the next minor version.strip-typesIssues or PRs related to strip-types support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@marco-ippolito@nodejs-github-bot@mcollina@legendecas@robpalme@statianzo@Bnaya@jakebailey@targos@ShogunPanda@jasnell@benjamingr@anonrig@khaosdoctor@magic-akari@himself65@aduh95@mertcanaltin@avivkeller@VoltrexKeyva