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

fix(core): fix custom eslint rules with typescript 5.2 and ts-node as transpiler #20387

Closed

Conversation

mattlewis92
Copy link
Contributor

Current Behavior

Using custom eslint rules fails when using ts-node + typescript 5.2 due to moduleResolution being set to node16 for the workspace rules tsconfig, but then the module value is always being overridden to commonjs by instead of being preserved as node16. This worked in previous versions of typescript, but from 5.2 this becomes a hard error: microsoft/TypeScript#54567

Which is then swallowed and results in all custom eslint rules failing with a message like this:

1:1 error Definition for rule '@nx/workspace/rule-name' was not found

Expected Behavior

Linting with custom workspace rules works with ts-node + typescript 5.2

Related Issue(s)

N/A

@mattlewis92 mattlewis92 requested a review from a team as a code owner November 23, 2023 12:14
Copy link

vercel bot commented Nov 23, 2023

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

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Feb 1, 2024 10:54am

@meeroslav meeroslav changed the title fix(linter): fix custom eslint rules with typescript 5.2 and ts-node as transpiler fix(core): fix custom eslint rules with typescript 5.2 and ts-node as transpiler Jan 30, 2024
mattlewis92 and others added 2 commits January 30, 2024 10:38
Co-authored-by: Miroslav Jonaš <missing.manual@gmail.com>
@mattlewis92
Copy link
Contributor Author

@meeroslav the CI failures are new after making this change, so I guess allowing setting the module option to anything would cause real breakages

CleanShot 2024-01-31 at 19 38 02

Should the e2e tests be updated, or should I revert the change and go back to the tighter check?

@JamesHenry
Copy link
Collaborator

Hi @mattlewis92 I'm really sorry for how long it's taken for you to get a response on this one.

This area of the codebase has changed a little bit since this PR was opened and I just tried to reproduce the issue you are describing and couldn't.

I created a new nx workspace (therefore using TS 5.4), added a custom lint rule, wired it up in my eslint config, and ensured it was running with ts-node (not swc) by setting NX_PREFER_TS_NODE=true and everything worked fine.

I then deliberately made the lint rule fail with a bad import to make sure it wasn't a false positive.

Please could you provide steps to reproduce the issue with the latest nx?

@mattlewis92
Copy link
Contributor Author

Hi @mattlewis92 I'm really sorry for how long it's taken for you to get a response on this one.

This area of the codebase has changed a little bit since this PR was opened and I just tried to reproduce the issue you are describing and couldn't.

I created a new nx workspace (therefore using TS 5.4), added a custom lint rule, wired it up in my eslint config, and ensured it was running with ts-node (not swc) by setting NX_PREFER_TS_NODE=true and everything worked fine.

I then deliberately made the lint rule fail with a bad import to make sure it wasn't a false positive.

Please could you provide steps to reproduce the issue with the latest nx?

All good, this has been fixed in the latest nx already by something else, we can close this :)

@mattlewis92 mattlewis92 closed this Jul 2, 2024
@JamesHenry
Copy link
Collaborator

Awesome, thank you for confirming!

Copy link

github-actions bot commented Jul 8, 2024

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants