-
-
Notifications
You must be signed in to change notification settings - Fork 24
feat: add TypeScript related extensions support #85
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
base: main
Are you sure you want to change the base?
Conversation
close #84 Signed-off-by: JounQin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 3214 3217 +3
=========================================
+ Hits 3214 3217 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for TypeScript-based configuration files by recognizing .cts
, .mts
, and .ts
extensions in the loader lookup.
- Introduces
.cts
,.mts
, and.ts
keys in theloaders
map to route TS files throughloadScriptOrModule
.
Comments suppressed due to low confidence (2)
lib/configuration.js:102
- Consider updating the README or configuration documentation to mention support for
.cts
,.mts
, and.ts
files so users know these extensions are now recognized.
'.cts': loadScriptOrModule,
lib/configuration.js:102
- Add or update tests to cover loading of
.cts
,.mts
, and.ts
configuration files, ensuring the new extensions are properly recognized in practice.
'.cts': loadScriptOrModule,
Hi team! I don’t know what’s up as there’s no phase label. Please add one so I know where it’s at. Thanks, |
I like it, but this needs to be documented. |
I thought it would only work with custom |
What should happen if both What happens in older Node, when this finds Is this really worth 3 extra |
@@ -99,6 +99,9 @@ const loaders = { | |||
'.cjs': loadScriptOrModule, | |||
'.mjs': loadScriptOrModule, | |||
'.js': loadScriptOrModule, | |||
'.cts': loadScriptOrModule, | |||
'.mts': loadScriptOrModule, | |||
'.ts': loadScriptOrModule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not come before .js
? Or is it intentional that built JS is preferred over TS
Next to docs, tests are missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think js configs should be preferred because they're supported natively and this is just old behavior, what means if a user have a .remarkrc.ts
compiled into .remarkrx.js
manually, this will continue work even on unsupported Node versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, probably!
It throws different errors compared with current behavior: parse TypeScript as json, and aftet this PR: parse as JavaScript failed. |
okay I’m up for this, if there are docs and tests. Looks like Remco is too. @ChristianMurphy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Considering the number of |
Good point while I think it should be discussed in another issue/PR instead? |
import type {Settings} from 'unified' | ||
|
||
exports.settings = { | ||
foo: 'bar' | ||
} satisfies Settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript doesn’t understand the exports
object inside TypeScript files. Instead, you are supposed to either use the special export =
syntax
import type {Settings} from 'unified' | |
exports.settings = { | |
foo: 'bar' | |
} satisfies Settings | |
import type {PresetSupportingSpecifiers} from 'unified' | |
const config: PresetSupportingSpecifiers = { | |
settings: { | |
foo: 'bar' | |
} | |
} | |
export = config |
or use an ESM style export (doesn’t work with verbatimModuleSyntax
)
import type {Settings} from 'unified' | |
exports.settings = { | |
foo: 'bar' | |
} satisfies Settings | |
import type {Settings} from 'unified' | |
export const settings: Settings = { | |
foo: 'bar' | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all features are supported by Node type stripping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, though I don’t know entirely for sure which features are or aren’t supported. If neither of these syntaxes is supported by Node.js type stripping, then it doesn’t support .cts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import type {Settings} from 'unified'
work well in .cts
, and .mts
also doesn't support all TypeScript features, both of them are partial supported, I treat .cts
same as .mts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider a module format supported, if there’s a way to import and export things in a way that TypeScript understands. In CJS, for imports means at least one of:
import module = require('module')
import {member} from 'module'
And for exports that means at least one of:
export = {}
export const member = {}
If there’s no (proper) way to import or export from a .cts
file, then it’s not properly supported. In that case IMO we should not support it either, as it promotes writing incorrect TypeScript code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case is still a valid .cts
usage, import
for typings, exports
for runtime codes.
We're talking about Node type stripping, and it's how .cts
works in Node itself which is partial support.
Using .cts
doesn't mean all module features are supported.
So IMO, .cts
is supported by Node itself this way, then we could support how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not valid .cts
usage. The use of the module
, exports
, or require
variables in .cts
is a hacky workaround. It only works, because the file isn’t imported by user code.
We shouldn’t promote this anti-pattern, especially considering this is already a topic of confusion in the TypeScript community.
Initial checklist
Description of changes
close #84
Note
It only works on Node 24+ for now: nodejs/node#57756 (comment)