Skip to content
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

[Bug]: ESM with TypeScript >= 5.0 hangs forever #2138

Closed
dgp1130 opened this issue Jul 10, 2023 · 12 comments · Fixed by #2197
Closed

[Bug]: ESM with TypeScript >= 5.0 hangs forever #2138

dgp1130 opened this issue Jul 10, 2023 · 12 comments · Fixed by #2197

Comments

@dgp1130
Copy link

dgp1130 commented Jul 10, 2023

Version

13.1.1 (current latest)

Steps to reproduce

Clone https://github.com/dgp1130/ng-large/tree/jest-preset-angular-esm-bug and run:

npm ci
npm test # Runs `jest`

There's nothing particularly special about this repo. It's basically a brand new ng new app on latest Angular where I have attempted to set up jest-preset-angular using ESM.

Expected behavior

Test should pass as there is only one no-op component in the repository.

Actual behavior

Jest prints:

RUNS src/app/app.component.spec.ts

And then hangs there forever, never making any meaningful progress.

Additional context

I'm using the current latest Angular, Jest, Node, and jest-preset-angular. Bisecting the versions a bit, I discovered that TypeScript 5.0 introduced the error. TS 4.9.4 does not reproduce, while everything greater than 5.0 (up through 5.1.4, the current latest) fails.

While Jest hangs without an error message, if you open a debugger with --expose-internals (npm run test:debug in that repo), I was able to find the failing assertion which includes the error:

Unhandled SyntaxKind: ImportClause.

I'm trying to use native ESM here, so I would think TypeScript should emit any imports as effectively a no-op and let Node handle the rest, so I'm not sure why TypeScript is failing to emit the import.

Environment

OS: Debian Linux
Node: v18.16.1
Jest: v29.6.1
@NicBright
Copy link

NicBright commented Sep 1, 2023

I've stumbled into this as well. Maybe my findings can help to find the underlying issue:

Part of the problem is related to jest-preset-angular's replace-resources.js adding a statement, presumably because it wants to add an import for the templateUrl: '...' part of the @Component() decorator

Here are my findings when debugging via inspect-brk and attaching to the debug session with chrome's about:inspect:

(1) see: it's the same error message that @dgp1130 observed

image

(2) going up the call stack I could very well follow a single node object that seems to have caused the error. I've discovered it has an escapedText of ___NG_CLI_RESOURCE__0 which made me think the bug is likely neither related to my sources, jest nor typescript. Instead it hints to some Angular library (i.e. I think it's jest-preset-angular).

image

(3) so I tried to find out where that faulty import clause comes from. Further up in the call stack I've discovered code that ran several scriptTransformers. (4) One of them being replace-resources.js

I've opened that file and searched for NG_CLI_RESOURCE which gave me a match (5)

(6) looking up createResourceImport() got me thinking it must have to do something with the (7)templateUrl of my Component (because the top level node is a SourceFileObject pointing to one of my components.

image

Unfortunately that's all I can say for now (and I don't have the time to continue investigations). It could be that jest-preset-angular is doing everything Ok and that maybe TypeScript introduced a regression with 5.0. Or maybe it has to do with some of the reported breaking changes of the TypeScript API in 5.0?

@platov
Copy link

platov commented Sep 11, 2023

@NicBright , you're right regarding TS 5.0 breaking changes. i looked into the source code of ts@5 compiler API and see the following createImportDeclaration function interface:

function createImportDeclaration(modifiers, importClause, moduleSpecifier, assertClause) {
 ...
}

Pay attention that the importClause accepts as second argument.

But in replace-resources.js file the createResourceImport function calls createImportDeclaration method incorrectly
by putting the importClause as 3rd argument.

...
const importDeclaration = nodeFactory.createImportDeclaration(undefined, undefined, nodeFactory.createImportClause(false, importName, undefined), urlLiteral);
...

When i manually removed second undefined argument of createImportClause call - the tests started to work normally.

@johncrim
Copy link
Contributor

johncrim commented Oct 1, 2023

We're experiencing the same issue - thank you for the investigations @dgp1130, @NicBright, and @platov . I'd done the same change isolation as @dgp1130 and @NicBright, then found this issue.

This issue has been blocking us upgrading to TS 5 in our repo for a few months, I hadn't had the time to isolate it until now.

johncrim added a commit to johncrim/ng16-jest-preset that referenced this issue Oct 2, 2023
@NicBright
Copy link

NicBright commented Nov 8, 2023

This issue is becoming increasingly urgent. It blocks us from upgrading to Angular 17 which requires TS >= 5.2. Any help would be welcome!

@platov
Copy link

platov commented Nov 8, 2023

@NicBright , I used "patch-package" npm lib to make the patch that applies automatically during npm i while this issue is not resolved by the jest-preset-angular authors.

@johncrim
Copy link
Contributor

I bet the team would take a PR...

@joeskeen
Copy link

joeskeen commented Dec 5, 2023

I'm assuming that any change to support TS5 would also have to maintain backwards compatibility with TS <5, right? I haven't looked into the code yet, but in theory If the code has any context of which TS version it's using, it could be a quick fix. With @platov's explanation above, I'd be happy to tackle it if someone could point me in the right direction. Also @platov if you could share the patch you are using with patch-package that would help a lot too :)

@johncrim
Copy link
Contributor

johncrim commented Dec 6, 2023

@joeskeen - I'm quite sure that's correct - it would have to work with TS 4.x in addition to TS 5 to be an acceptable fix. I haven't looked into this very much (besides a bunch of debugging/isolating the problem), but I do see:

https://github.com/thymikee/jest-preset-angular/blob/main/src/ngtsc/ts_compatibility/src/ts_cross_version_utils.ts

which looks like a way to make TS calls compatible across versions.

@ahnpnl - any pointers for @joeskeen on this?

@platov
Copy link

platov commented Dec 12, 2023

I'm assuming that any change to support TS5 would also have to maintain backwards compatibility with TS <5, right? I haven't looked into the code yet, but in theory If the code has any context of which TS version it's using, it could be a quick fix. With @platov's explanation above, I'd be happy to tackle it if someone could point me in the right direction. Also @platov if you could share the patch you are using with patch-package that would help a lot too :)

Patch example for jest-preset-angular+13.1.1. Put this file in patches folder of the project root.
jest-preset-angular+13.1.1.patch

@johncrim
Copy link
Contributor

johncrim commented Jan 2, 2024

@ahnpnl - could you please comment on this issue? It's blocking typescript and angular upgrades for any project using jest-preset-angular, and you and/or @thymikee have been conspicuously silent on it for six months, whereas you have been extremely generous, responsive, and helpful in the past. Even if you stated "we do not intend to fix this" or "I no longer have time to donate to this project" or "I don't see a good way to fix this without breaking compatibility", that would be helpful to know for users of this project.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 2, 2024

One possible thing is because we relies on kulshekhar/ts-jest#4226

@johncrim
Copy link
Contributor

johncrim commented Jan 2, 2024

Thank you for the reply @ahnpnl .

I am not familiar with all the layers in jest-preset-angular and ts-jest, but I believe this issue is not the same, though the symptoms are the same; I believe the problem here is described accurately by @platov as a breaking change in the TS 5 API. This breaking change could certainly affect both projects, but the issues look different to me.

I have not tested @platov 's patch, but I will later today.

johncrim added a commit to johncrim/jest-preset-angular that referenced this issue Jan 3, 2024
This update appears to fix thymikee#2138. It builds with TS 5, and all tests and example tests pass.
johncrim added a commit to johncrim/jest-preset-angular that referenced this issue Jan 3, 2024
This update appears to fix thymikee#2138. It builds with TS 5, and all tests and example tests pass.

fixes: [Bug]: ESM with TypeScript >= 5.0 hangs forever thymikee#2138
ahnpnl pushed a commit that referenced this issue Jan 3, 2024
* fix(ts5): Update build dependencies and types for TS 5

This update appears to fix #2138. It builds with TS 5, and all tests and example tests pass.

fixes: [Bug]: ESM with TypeScript >= 5.0 hangs forever #2138

* chore(deps): Add Husky dev dependency

Husky is required by the postinstall script, but was previously missing.

* chore(upgrade ng): Upgrade to ng17 latest

Tests pass, and example tests pass.

resolves: [Feature]: Angular 17 support  #2196

* chore(dedupe): yarn dedupe

* fix(eslint): Disable eslint warnings due to use of "any"

* chore(deps): Change dev dependency for TS to match @angular/compiler restrictions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants