Skip to content

feat(devkit): allow requiring cts config files #31103

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

Merged
merged 5 commits into from
May 16, 2025

Conversation

nicolas-marien
Copy link
Contributor

@nicolas-marien nicolas-marien commented May 7, 2025

When migrating our project to esm, we encountered an issue with the playright plugin, but more generally with the loadConfigFile from the devkit.
Our configuration is a .cts file, but it's not treated as commonjs: __dirname and __filename are not available.

CleanShot 2025-05-07 at 15 31 04@2x

Expected Behavior

.cts files are interpreted as commonJS files when in a module context.

@nicolas-marien nicolas-marien requested a review from a team as a code owner May 7, 2025 13:32
Copy link

vercel bot commented May 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview May 16, 2025 2:36pm

Copy link

nx-cloud bot commented May 7, 2025

View your CI Pipeline Execution ↗ for commit e664514.

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 37m 26s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 18s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 2s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 4s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 3s View ↗
nx documentation ✅ Succeeded 2m 45s View ↗

☁️ Nx Cloud last updated this comment at 2025-05-16 15:40:29 UTC

Copy link
Member

@jaysoo jaysoo left a comment

Choose a reason for hiding this comment

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

I think if we know that the file is specifically .cts or .mts, we can choose either require or dynamic import without having to do what load function currently does (which is try one, then try the other).

@nicolas-marien
Copy link
Contributor Author

@jaysoo thank you for the feedback. I'll update the code accordingly.

@FrozenPandaz FrozenPandaz added the priority: high High Priority (important issues which affect many people severely) label May 9, 2025
@jaysoo
Copy link
Member

jaysoo commented May 16, 2025

I tested this locally. I had to make the mts use load instead of loadEsm as there might be some CJS specific contents in there like __filename. Depending on tsconfig.base.json, the modules may be transpiled as CJS anyway.

This PR now fixes the .cts issue, while maintaining same support for .ts, .mts. And for mjs, it'll go straight to loading via ESM.

@jaysoo jaysoo merged commit bd88b0e into nrwl:master May 16, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high High Priority (important issues which affect many people severely)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants