fix(linter): use swc over ts-node for workspace lint rule registration #8694
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nrwl/nx-dev/FCn7HRgixr6GnDMM2p5mVZ8SCx8d [Deployment for 66f09d0 canceled] |
There was a problem hiding this comment.
I think we can also replace tsconfig-paths with native swc-node capabilities based on seeing this PR and the issue it closed: swc-project/swc-node#608
Please could you explore that before we finalize this change?
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
707b370
to
3ba1b83
Compare
Turns out |
|
@meeroslav cool just AFK right now but do we have an e2e test which covers using a workspace lib in rule to ensure it is working as before? The tsconfig-paths dep is still in there FYI, it can probably be removed now |
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
3ba1b83
to
c524e0d
Compare
|
@JamesHenry, I removed the We do have an E2E test which checks if the workspace-lint is working. Additionally, I used this rule to test if the using workspace projects and nrwl/* packages in rules work. Is there something else that should be checked? |
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
c524e0d
to
1f3cf7a
Compare
There was a problem hiding this comment.
Yes, we need to ensure that we can users can use utilities from their own Nx workspaces in their lint rules.
I have done a local build using this branch in order to create a workspace and that use-case is no longer working after this change, I'll ping you to walk you through it
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
9368408
to
5518077
Compare
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
5518077
to
b4bc01a
Compare
| try { | ||
| /** | ||
| * Currently we only support applying the rules from the user's workspace plugin object | ||
| * (i.e. not other things that plugings can expose like configs, processors etc) | ||
| */ | ||
| const { rules } = require(WORKSPACE_PLUGIN_DIR); | ||
|
|
||
| if (registrationCleanup) { |
There was a problem hiding this comment.
Thinking out loud, maybe this should come in a finally {} block so that it still happens in the error case? Not sure how much leakage is likely to occur with this though, so maybe it's not actually not an issue...
There was a problem hiding this comment.
It's very unlikely that registration succeeds but requiring the project fails, but I guess it's better be safe than sorry.
In the previous version we never did the cleanup at all, and we never noticed any leaks.
I realised that by doing the cleanup we save few hundred milliseconds per run. I haven't had time to investigate why.
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
3660564
to
d1bf12b
Compare
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
d1bf12b
to
ca76c20
Compare
e2e/linter/src/linter.test.ts
Outdated
| @@ -315,7 +335,32 @@ function updateGeneratedRuleImplementation( | |||
|
|
|||
| return ts.visitEachChild(node, visit, context); | |||
| } | |||
| return ts.visitNode(rootNode, visit); | |||
| /** | |||
| * Add lib import as a first line of the rule file | |||
There was a problem hiding this comment.
Please could we add the "why" behind this for the benefit of other engineers who weren't directly involved in this PR? It might not be clear why we felt this was important
meeroslav
force-pushed
the
fix/linter-7758-2
branch
from
ca76c20
to
66f09d0
Compare
|
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. |
Current Behavior
Expected Behavior
Related Issue(s)
Fixes #7758