Use tsx for parsing typescript sources#771
Use tsx for parsing typescript sources#771noseworthy wants to merge 2 commits intonode-config:masterfrom
Conversation
`ts-node` has issues working with `esm`, so switch it out in favour of using `tsx` which just works "out of the box".
|
I'm not sure if this is something you'd consider @lorenwest, but I was having issues working with TypeScript and |
|
Thank you for bringing this to our attention. That sounds like a healthy option to support. Do you have any ideas about how an installation would choose It has to run with current installations, so the default may have to be |
|
So that's why I'm not super dedicated to this approach. Maybe this can be adapted so that I'm not super sure what the best approach is as I don't have much history with this project and wouldn't want to introduce something that's against the goals/style here. Thanks for taking a look though, and happy to bring |
|
Leaving open if any contributors want to take on this issue. |
|
I am not familiar with tsx and am neutral on this proposal. |
Is that a bad thing in a config library? Least Power suggests to me that reflection in load time logic might be a bridge too far. I could see a case for this being a wontfix. Do you know if this plays nice with nyc? If so then I’m on board. Does it make #740 worse, better, indifferent? Could you take a stab at updating the readme to reflect this caveat? |
Sadly, no.
|
|
I was hoping for advice from the Istanbul people but no luck so far |
|
See #847 |
|
Depending on how this ticket plays out, we may need to overlook the downsides of this solution. |
|
FYI, just ran into an issue (I belive nodejs/node#59364 referenced above) when trying to use native ts support after upgrading node from 22.17 to 22.18. I load Error: Cannot parse config file: '<PROJECT_PATH>/config/default.ts': Error: Must use import to load ES Module: <PROJECT_PATH>/config/default.ts
require() of ES modules is not supported.
require() of <PROJECT_PATH>/config/default.ts from <PROJECT_PATH>/node_modules/config/parser.js is an ES module file as it is a .ts file whose nearest parent package.json contains "type": "module" which defines all .ts files in that package scope as ES modules.
Instead change the requiring code to use import(), or remove "type": "module" from <PROJECT_PATH>/package.json.I made a wrapper import c from './defaultConfig.ts';
export default c;This avoids Maybe you could somehow work this into your lib? |
|
@Properko You're trying to use native mode rather than ts-node, right? I took a poke at if the type erasure stuff works with node-config and that's a big negatory, good buddy. All the examples show problems with import statements that require syntax that only works in NodeJS but is a syntax error in all versions of ts-node (I checked) and in my IDE, which is not a good sign. I don't know if the documentation is wrong. I don't have a TS project at hand to test this with. |
|
@jdmarshall right. I'd been using node to run typescript with experimental flags and it was unflagged in 22.18. At the same time something broke with ts-node in 22.18 as I've posted above. So to avoid config using ts-node and crashing on import, my workaround is to make the OTOH, the native typescript is not supported in node_modules (to prevent lib maintainers from publishing raw typescript), so I'm not sure whether the config library can rely on having this capability built-in to node or if it would need to depend on and use amaro explicitly. |
|
That said, this PR with ^ actually nvm, most of my suggestions are based on type stripping, while you support typescript fully including enums etc. which is still flagged in node. |
📝 WalkthroughWalkthroughDependency migration replacing ts-node with tsx in package.json and updating the TypeScript runtime module binding from 'ts-node' to 'tsx/cjs/api' in parser.js configuration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 79: parser.js currently hardcodes require('tsx/cjs/api') which breaks
users who only have ts-node; modify the TS runtime resolution in parser.js (the
require logic that loads 'tsx/cjs/api') to first attempt requiring 'ts-node'
inside a try/catch and fall back to requiring 'tsx/cjs/api' if that fails, and
also add support for an environment variable (e.g., TS_RUNTIME) that, when
present, forces require of that module name instead of the default resolution
order; ensure the change is implemented where the TS runtime is loaded so tests
still use tsx but existing users with ts-node are supported.
In `@parser.js`:
- Line 35: tsParser is calling TS_DEP.register() with ts-node-specific options
that tsx's register() doesn't accept, so those options are silently ignored;
locate the TS_DEP constant and the tsParser function where register() is invoked
and remove the ts-node options (lazy, ignore, transpileOnly, compilerOptions)
from the call, updating the call to either TS_DEP.register() or
TS_DEP.register({ namespace: '...' }) to match tsx's API, and if any behavior
from those options is required, implement the equivalent via tsx
configuration/environment or another supported API rather than passing ts-node
options to register().
| "semver": "5.3.0", | ||
| "toml": "^2.0.6", | ||
| "ts-node": "^10.9.2", | ||
| "tsx": "^4.17.0", |
There was a problem hiding this comment.
Breaking change for users with ts-node installed.
Replacing ts-node with tsx in devDependencies is fine for the project's own tests, but since parser.js now hardcodes tsx/cjs/api as the TS runtime dependency, any user who currently has ts-node installed (but not tsx) will get a runtime error when loading .ts config files. The PR discussion suggested a fallback strategy (try ts-node first, fall back to tsx, or vice versa) but none was implemented.
Consider either:
- Attempting
require('ts-node')first with atry/catchfallback torequire('tsx/cjs/api'), or - Allowing users to configure the TS parser via an environment variable.
This would preserve backward compatibility while adding tsx support.
🤖 Prompt for AI Agents
In `@package.json` at line 79, parser.js currently hardcodes
require('tsx/cjs/api') which breaks users who only have ts-node; modify the TS
runtime resolution in parser.js (the require logic that loads 'tsx/cjs/api') to
first attempt requiring 'ts-node' inside a try/catch and fall back to requiring
'tsx/cjs/api' if that fails, and also add support for an environment variable
(e.g., TS_RUNTIME) that, when present, forces require of that module name
instead of the default resolution order; ensure the change is implemented where
the TS runtime is loaded so tests still use tsx but existing users with ts-node
are supported.
| PPARSER_DEP = 'properties', | ||
| XML_DEP = 'x2js', | ||
| TS_DEP = 'ts-node'; | ||
| TS_DEP = 'tsx/cjs/api'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate parser.js and examine lines 35 and 93-102
find . -name "parser.js" -type f | head -5Repository: node-config/node-config
Length of output: 78
🌐 Web query:
tsx/cjs/api register function API documentation
💡 Result:
The tsx/cjs/api CommonJS Register API is documented here: “CommonJS Register API”. (tsx.is)
What register() does (CJS):
const tsx = require('tsx/cjs/api')const unregister = tsx.register()enables requiring TypeScript files in CommonJS (e.g.require('./file.ts')), andunregister()disables it again. (tsx.is)
Scoped / namespaced registration:
const api = tsx.register({ namespace: '...' })- Then load TS via
api.require('./file.ts', __filename)and clean up withapi.unregister(). (tsx.is)
Type/overload summary (from tsx’s packaged docs):
register(options?: RegisterOptions): Unregisterregister({ namespace: string, ... }: RegisterOptions): NamespacedUnregister(returns an object that includesrequire,resolve, andunregister). (tessl.io)
Citations:
- 1: https://tsx.is/dev-api/register-cjs
- 2: https://tsx.is/dev-api/register-cjs
- 3: https://tsx.is/dev-api/register-cjs
- 4: https://tessl.io/registry/tessl/npm-tsx/4.20.0/files/docs/cjs-api.md?utm_source=openai
🏁 Script executed:
# Read parser.js lines 30-40 to see line 35
sed -n '30,40p' parser.js
# Also read lines 90-105 to see the tsParser function
sed -n '90,105p' parser.jsRepository: node-config/node-config
Length of output: 728
🏁 Script executed:
# Let me verify the complete tsParser function to see the full scope
sed -n '90,110p' parser.jsRepository: node-config/node-config
Length of output: 710
tsx/cjs/api.register() does not accept ts-node options — configuration will be silently ignored.
The TS_DEP was changed to tsx/cjs/api (line 35), but tsParser (lines 93–102) still calls .register() with ts-node-specific options (lazy, ignore, transpileOnly, compilerOptions). According to tsx documentation, register() only accepts an optional { namespace: string } parameter. The ts-node options will be silently ignored, causing the TypeScript registration to behave differently than intended.
The registration call should be updated to match tsx's API:
Proposed fix
Parser.tsParser = function(filename, content) {
if (!require.extensions['.ts']) {
- require(TS_DEP).register({
- lazy: true,
- ignore: ['(?:^|/)node_modules/', '.*(?<!\.ts)$'],
- transpileOnly: true,
- compilerOptions: {
- allowJs: true,
- }
- });
+ require(TS_DEP).register();
}🤖 Prompt for AI Agents
In `@parser.js` at line 35, tsParser is calling TS_DEP.register() with
ts-node-specific options that tsx's register() doesn't accept, so those options
are silently ignored; locate the TS_DEP constant and the tsParser function where
register() is invoked and remove the ts-node options (lazy, ignore,
transpileOnly, compilerOptions) from the call, updating the call to either
TS_DEP.register() or TS_DEP.register({ namespace: '...' }) to match tsx's API,
and if any behavior from those options is required, implement the equivalent via
tsx configuration/environment or another supported API rather than passing
ts-node options to register().
ts-nodehas issues working withesm, so switch it out in favour of usingtsxwhich just works "out of the box".Summary by CodeRabbit