-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: restore typescript 5.6 compatibility #301
Conversation
@@ -135,7 +135,7 @@ export default function transformer( | |||
...tsTransformPathsContext, | |||
sourceFile, | |||
isDeclarationFile: sourceFile.isDeclarationFile, | |||
originalSourceFile: (<typeof ts>tsInstance).getOriginalSourceFile(sourceFile), | |||
originalSourceFile: ts.getParseTreeNode(sourceFile, ts.isSourceFile) || sourceFile, |
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.
See PR description - both of these methods are not marked as @internal
.
This is the exact implementation of getOriginalSourceFile
: https://github.com/microsoft/TypeScript/blob/c8a7d589e647e19c94150d9892909f3aa93e48eb/src/compiler/utilities.ts#L5416-L5419
["5.6.3", tsFiveSix, "typescript-5.6"], | ||
["Latest", ts, "typescript"], |
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.
Technically, 5.6.3 and latest
are the same thing right now. However, I wanted to explicitly define 5.6.3 so we run tests against it. Then, when 5.7++ come out, we'll get test coverage.
@@ -34,14 +34,20 @@ describe(`Extra Tests`, () => { | |||
}); | |||
|
|||
describe(`ts-node register script`, () => { | |||
/** Yarn sometimes outputs bold text, which makes these tests flakey */ |
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.
These tests were failing for me when I ran them locally. The output of yarn
seems to include control characters to bold the string null
, which is why I'm stripping the string with this method.
FAIL tests/extras.test.ts (8.413 s)
● Extra Tests › Built Tests › ts-node register script › Works with --transpileOnly
expect(received).toMatch(expected)
Expected pattern: /^null($|\r?\n)/m
Received string: "null
"
37 | test(`Works with --transpileOnly`, () => {
38 | const res = execSync("yarn g:ts-node --transpileOnly src/index.ts", { cwd: projectRoot }).toString();
> 39 | expect(res).toMatch(/^null($|\r?\n)/m);
| ^
40 | });
41 |
42 | test(`Works with --typeCheck`, () => {
at Object.<anonymous> (tests/extras.test.ts:39:21)
● Extra Tests › Built Tests › ts-node register script › Works with --typeCheck
expect(received).toMatch(expected)
Expected pattern: /^null($|\r?\n)/
Received string: "null
"
42 | test(`Works with --typeCheck`, () => {
43 | const res = execSync("yarn g:ts-node --typeCheck src/index.ts", { cwd: projectRoot }).toString();
> 44 | expect(res).toMatch(/^null($|\r?\n)/);
| ^
45 | });
46 | });
47 | });
at Object.<anonymous> (tests/extras.test.ts:44:21)
test(`Works with --transpileOnly`, () => { | ||
const res = execSync("yarn g:ts-node --transpileOnly src/index.ts", { cwd: projectRoot }).toString(); | ||
expect(res).toMatch(/^null($|\r?\n)/m); | ||
expect(stripAnsi(res.trim())).toEqual("null"); |
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.
Also, instead of caring about the newline (which can be different between windows and mac), I just .trim()
it.
Thank you for the contribution @blimmer. Overall LGTM. I'll take some time this week to try this out, give it a proper review&test and see if we can get it merge. |
This PR restores compatibility with TypeScript 5.6. It also updates the test suite to be more resilient and explicitly tests against TypeScript 5.5 and 5.6.
I was surprised that TypeScript would break this without releasing a major version. However, if you look at the
getOriginalSourceFile
method we were using, it was marked as@internal
in the JSDoc: https://github.com/microsoft/TypeScript/blob/c8a7d589e647e19c94150d9892909f3aa93e48eb/src/compiler/utilities.ts#L5416-L5419Because it was marked as internal, they probably reserved the right to remove this method without a breaking version bump. The other PR to fix this issue (#270) suggested using
getSourceFileOfNode
, but that's also marked as@internal
: https://github.com/microsoft/TypeScript/blob/11b2930fa2c9f73b0ffb725a9715b8d3c4121bbc/src/compiler/utilities.ts#L965-L975.Therefore, instead, I updated this code to use
getParseTreeNode
andisSourceFile
, which are both exposed publicly and not marked asinternal
: https://github.com/microsoft/TypeScript/blob/11b2930fa2c9f73b0ffb725a9715b8d3c4121bbc/src/compiler/utilitiesPublic.ts#L803-L809 / https://github.com/microsoft/TypeScript/blob/11b2930fa2c9f73b0ffb725a9715b8d3c4121bbc/src/compiler/factory/nodeTests.ts#L1013-L1016By using these methods, we should be more resilient to internal refactoring.
This PR supersedes #270.
Fixes #266