Skip to content

Update file extensions for Node TS support #6596

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.

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

Closed
wants to merge 7 commits into from
Closed

Conversation

RDIL
Copy link
Member

@RDIL RDIL commented Nov 12, 2024

What's the problem this PR addresses?

Fixes #6595

How did you fix it?

Always consider mts as ESM and cts as CJS

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@adrian-gierakowski
Copy link

Anything stopping progress on this PR?

@RDIL RDIL changed the title Treat .mts as an es module file Update file extensions for Node TS support Jan 20, 2025
@RDIL RDIL marked this pull request as ready for review January 20, 2025 15:34
@RDIL RDIL requested a review from merceyz January 20, 2025 15:34
@RDIL
Copy link
Member Author

RDIL commented Jan 20, 2025

Sorry @adrian-gierakowski, completely forgot to follow up here.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

This PR needs tests to ensure it works and that we don't regress on it.

@@ -20,3 +20,6 @@ export const SUPPORTS_IMPORT_ATTRIBUTES = major >= 21 || (major === 20 && minor

// https://github.com/nodejs/node/pull/52104
export const SUPPORTS_IMPORT_ATTRIBUTES_ONLY = major >= 22;

// https://github.com/nodejs/node/pull/53725
export const SUPPORTS_TYPE_STRIPPING = major > 23 || (major === 23 && minor > 6);
Copy link
Contributor

Choose a reason for hiding this comment

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

to make strip types works, you need to change pnp mjs loader and chain other loaders , because (assumption is) types stripes is implemented as loader also.
#6595 (comment)

Copy link
Contributor

@goloveychuk goloveychuk Jan 27, 2025

Choose a reason for hiding this comment

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

test with simple type cast ( as any) could written to validates this

@goloveychuk
Copy link
Contributor

@RDIL can we finish this pls?

@goloveychuk
Copy link
Contributor

return {
    format,
    source: format === `commonjs` ? void 0 : await fs.promises.readFile(filePath, `utf8`),
    shortCircuit: false
  };

should be replaced with return nextLoad(urlString, context, nextLoad);
because typescript types striping is done by loader.
So, probably, pnp loader should hack fs and and then just chain other loaders without explicit loading files.
@arcanis wdyt?

@RDIL
Copy link
Member Author

RDIL commented Apr 10, 2025

@goloveychuk sorry, i'm super busy these days, but if you want to grab my commits and make a new PR, I'm happy to review

@RDIL
Copy link
Member Author

RDIL commented Apr 13, 2025

Closing in favor of #6763

@RDIL RDIL closed this Apr 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: ERR_REQUIRE_ESM in Yarn PnP mode using Node.js 22 with *.mts file and --experimental-strip-types flag
4 participants