nx: ESLint Plugin: mixing dynamic and static imports forbidden for different imports to the same module
Current Behavior
We currently have a package with multiple entry points. For example, one part of the package can be required like
forivall/nx-repro-local-plugin-issue@ c0247e2/packages/demo/src/index.ts#L1
import staticDep from '@repro-local-plugin-issue/dep';
And another part can be required like
forivall/nx-repro-local-plugin-issue@ c0247e2/packages/demo/src/lib/demo.ts#L2
return (await import('@repro-local-plugin-issue/dep/dynamic')).default;
When I run nx lint demo on the reproduction repo, it reports
packages/demo/src/index.ts
1:1 error Imports of lazy-loaded libraries are forbidden @nrwl/nx/enforce-module-boundaries
Expected Behavior
To resolve the eslint error, either the ESLint rule should detect that these are from different import specifiers, or the rule should be able to be disabled.
GitHub Repo
https://github.com/forivall/nx-repro-local-plugin-issue
Steps to Reproduce
npm installnx lint demo
Nx Report
Node : 16.17.0
OS : darwin arm64
npm : 8.15.0
nx : 15.8.5
@nrwl/js : 15.8.5
@nrwl/jest : 15.8.5
@nrwl/linter : 15.8.5
@nrwl/workspace : 15.8.5
@nrwl/cli : 15.8.5
@nrwl/devkit : 15.8.5
@nrwl/eslint-plugin-nx : 15.8.5
@nrwl/nx-plugin : 15.8.5
@nrwl/tao : 15.8.5
typescript : 4.9.5
---------------------------------------
Local workspace plugins:
@repro-local-plugin-issue/repro
Failure Logs
» nx lint demo --verbose
> nx run demo:lint
Linting "demo"...
/Users/emily.klassen/code/sandbox/repro-local-plugin-issue/packages/demo/src/index.ts
1:1 error Imports of lazy-loaded libraries are forbidden @nrwl/nx/enforce-module-boundaries
✖ 1 problem (1 error, 0 warnings)
Lint errors found in the listed files.
————————————————————————————————————————————————————————————————————
> NX Running target lint for project demo failed
Failed tasks:
- demo:lint
Hint: run the command with --verbose for more details.
Additional Information
Originally reported at https://github.com/nrwl/nx/issues/15372#issuecomment-1457413733
nx 15.7 gives the following dependency graph:
$ nx graph --file graph-15_7.json && jq .graph.dependencies
> NX JSON output created in /Users/emily.klassen/code/sandbox/repro-local-plugin-issue
/Users/emily.klassen/code/sandbox/repro-local-plugin-issue/graph-15_7.json
{
"repro-e2e": [
{
"source": "repro-e2e",
"target": "repro",
"type": "implicit"
}
],
"repro": [],
"demo": [
{
"source": "demo",
"target": "dep",
"type": "static"
}
],
"dep": []
}
but nx 15.8 gives the following:
» nx graph --file graph-15_8.json && jq .graph.dependencies graph-15_8.json
> NX JSON output created in /Users/emily.klassen/code/sandbox/repro-local-plugin-issue
/Users/emily.klassen/code/sandbox/repro-local-plugin-issue/graph-15_8.json
{
"repro-e2e": [
{
"source": "repro-e2e",
"target": "repro",
"type": "implicit"
}
],
"repro": [],
"demo": [
{
"source": "demo",
"target": "dep",
"type": "static"
},
{
"source": "demo",
"target": "dep",
"type": "dynamic"
}
],
"dep": []
}
My current workaround is to use // nx-ignore-next-line on the dynamic import statements.
About this issue
- Original URL
- State: closed
- Created a year ago
- Reactions: 4
- Comments: 19 (6 by maintainers)
Does this case make sense? Note that the lib is generated with @nrwl/next There are 2 libs (LibA and LibB).
LibA contains a Button component LibB contains a Card and a Modal component
This will also triggers
Imports of lazy-loaded libraries are forbidden @nrwl/nx/enforce-module-boundariesinCardAgain, this is exclusively Nextjs stuff that you are discussing here and it’s not something I have huge expertise in. Nx is not Nextjs and works with many, many technologies, so this rule exists to protect against common bundling footguns.
If Next is able to help optimize some of these inconsistent loading approaches to the same code, then that’s good for their users, but you are also highlighting here that you are offloading code to the first load. There is no free lunch I don’t think.
Nx’s focus is on helping repos scale effectively and efficiently, whilst supporting any possible mixture of dozens of technologies. The clearest way to approach your problem would be to separate dynamically loaded code from statically loaded code.
You don’t have to do that, though - lint rules are there to try and help you, but you are in control. You can always use ESLint ignore comments on particular imports you feel are safe from the issues it is trying to protect you against. But as a toolset we need to err on the side of protecting the broadest possible set of our users.
Hey all,
Previously we had a bug in our codebase which treated dynamic dependencies as static ones, so this issue was not surfacing.
Generally, it should not be allowed to directly import a project (as a static dependency) and lazy load it since lazy loading would make no sense.
Unfortunately, for arbitrary imports, we can’t tell if those two files have a clean separation between them or if there is some chain of dependencies. E.g.
The
import A from 'a'andconst b = await import('@myrepo/b').defaultmake sense as long as there is no filecwhere b imports c, and c imports a.I think the best thing is to:
// eslint-disable-next-line @nrwl/nx/enforce-module-boundariesto cancel check orI’m also experiencing this issue.
@boompikachu When you statically import the code will already be present on every page in your application, because it is in your application bundle. pageA vs pageB does not impact this. So lazy loading on one page or another doesn’t help because the code is already there in every case.
You would need to ensure that you dynamically load the code consistently everywhere you reference it, in order to get the benefits of lazy-loading.
Please note that all of these points are exclusively about “how does Nextjs work”. I would recommend that you check out the guides and documentation from that ecosystem/community to gain a deeper understanding. The official website is natural a good starting point: https://nextjs.org/
The only role Nx is playing (via ESLint) in your case is to help prevent the intention behind the lazy-loading being invalidated, which it is doing appropriately in this case.
Reposting my response from the PR:
I can’t help feeling like this is too strong… Maybe we should instead allow an array of exceptions to be passed? So that certain patterns like the
@foo/bar/testingcan be enabled without sacrificing the general protection of this rule?Not easy to name the option but maybe something like:
The downside of losing the protection is end user-facing, so that’s why I feel like we need to try and ensure folks are not giving up on this lightly…
Using
nx-ignore-next-lineoverrides the check on the graph generation level (influencing how the graph deps are constructed) while// eslint-disable-next-line @nrwl/nx/enforce-module-boundariesonly influences the Linter rule.My suggestion would be to use the latter if possible.
Same for me. We should put on each libs affected import
// eslint-disable-next-line @nrwl/nx/enforce-module-boundariesas workaround but we prefer to wait for a fix. Thanks!