Skip to content
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.

Sign up for GitHub

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

Jump to bottom

module: add --experimental-transform-types flag #54283

Merged
merged 2 commits into from
Aug 12, 2024
Merged

module: add --experimental-transform-types flag #54283

merged 2 commits into from
Aug 12, 2024

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Aug 9, 2024 •
edited
Loading

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-ippolito marco-ippolito changed the title module: add --experimental-enable-transformation for strip-types [WIP] module: add --experimental-enable-transformation for strip-types Aug 9, 2024
@nodejs-github-bot nodejs-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-ippolito marco-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-ippolito marco-ippolito marked this pull request as ready for review August 9, 2024 11:20
@marco-ippolito marco-ippolito changed the title [WIP] module: add --experimental-enable-transformation for strip-types module: add --experimental-enable-transformation for strip-types Aug 9, 2024
@marco-ippolito marco-ippolito removed the wip Issues and PRs that are still a work in progress. label Aug 9, 2024
mcollina
mcollina approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina 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-ippolito
marco-ippolito commented Aug 9, 2024
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 9, 2024
@marco-ippolito marco-ippolito force-pushed the feat/enable-transformations branch 2 times, most recently from de62233 to 7343717 Compare August 9, 2024 12:15
legendecas
legendecas reviewed Aug 9, 2024
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
legendecas
legendecas reviewed Aug 9, 2024
View reviewed changes
Copy link
Member

@legendecas legendecas 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?

khaosdoctor
khaosdoctor reviewed Aug 9, 2024
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito changed the title module: add --experimental-enable-transformation for strip-types module: add --experimental-enable--type-transform for strip-types Aug 9, 2024
@marco-ippolito marco-ippolito changed the title module: add --experimental-enable--type-transform for strip-types module: add --experimental-enable-type-transform for strip-types Aug 9, 2024
mcollina
mcollina requested changes Aug 9, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment •
edited
Loading

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
Member Author

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-ippolito marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
@nodejs-github-bot nodejs-github-bot merged commit 0301309 into nodejs:main Aug 12, 2024
61 checks passed
@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
Member Author

marco-ippolito commented Aug 12, 2024 •
edited
Loading

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

@RyanCavanaugh RyanCavanaugh mentioned this pull request Aug 12, 2024
Open
6 tasks
@Bnaya
Copy link

Bnaya commented Aug 13, 2024

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

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
@marco-ippolito @targos
module: add --experimental-transform-types flag
945b1ca 
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
@marco-ippolito @targos
module: add --experimental-transform-types flag
abdb925 
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 targos added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Aug 14, 2024
@targos
Copy link
Member

targos commented Aug 14, 2024

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
Member Author

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 marco-ippolito mentioned this pull request Aug 14, 2024
Closed
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Aug 14, 2024
@marco-ippolito
module: add --experimental-transform-types flag
3186730 
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]>
@justinfagnani justinfagnani mentioned this pull request Aug 15, 2024
Open
@allisonkarlitskaya allisonkarlitskaya mentioned this pull request Aug 16, 2024
Open
@marco-ippolito marco-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
@marco-ippolito @RafaelGSS
module: add --experimental-transform-types flag
3eab8ce 
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
@RafaelGSS
2024-08-20, Version 22.7.0 (Current)
56a5662 
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 RafaelGSS mentioned this pull request Aug 20, 2024
Merged
@marco-ippolito marco-ippolito mentioned this pull request Aug 20, 2024
Open
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
@marco-ippolito @RafaelGSS
module: add --experimental-transform-types flag
240d929 
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
@RafaelGSS
2024-08-22, Version 22.7.0 (Current)
65eff1e 
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
@RafaelGSS
2024-08-22, Version 22.7.0 (Current)
4d5c885 
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
@ShenHongFei ShenHongFei mentioned this pull request Sep 3, 2024
Closed
@targos targos 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments

@magic-akari magic-akari magic-akari left review comments

@jakebailey jakebailey jakebailey left review comments

@RedYetiDev RedYetiDev RedYetiDev left review comments

@mertcanaltin mertcanaltin mertcanaltin approved these changes

@legendecas legendecas legendecas approved these changes

@khaosdoctor khaosdoctor khaosdoctor approved these changes

@anonrig anonrig anonrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@mcollina mcollina mcollina approved these changes

@himself65 himself65 himself65 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. strip-types Issues or PRs related to strip-types support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.