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

  1. npm install
  2. nx 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)

Most upvoted comments

@sigginjals, if you load this dependency on one page (within the same app) statically, then the bundler will include that entire package in the bundle. Calling lazy loading on another page would have no impact since the entire package is already in the bundle.

Your explanation only makes sense to me in the case of micro frontends, where each page is a separate app, but in that case, you won’t see this error.

There is a third scenario, explained by @forivall, where the package has multiple entry points. If we load just one slice statically, the bundler (hopefully) tree shakes other parts away. And then, we dynamically load just other slices. This is, of course, hard to verify in standard packages. The angular has a solution for this with standardized secondary entry points using ng-packagr, and for this, we have support in our rule. But each entry point has a separate path in tsconfig and we use the configuration to verify they do belong to different slices.

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

// LibA Button
const Button = (props) => <button {...props}>click me</button>
// LibB Card
import {Button} from '@project/lib-a' // Error here

const Card = () => <div><Button /></div>
// LibB Modal
import dynamic from 'next/dynamic'

const Button = dynamic(() => import('@project/lib-a').then(mod => mod.Button))

const Modal = () => <div><Button /></div>

This will also triggers Imports of lazy-loaded libraries are forbidden @nrwl/nx/enforce-module-boundaries in Card

Again, 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' and const b = await import('@myrepo/b').default make sense as long as there is no file c where b imports c, and c imports a.

I think the best thing is to:

  • Use explicit // eslint-disable-next-line @nrwl/nx/enforce-module-boundaries to cancel check or
  • Adding a custom flag to config that would disable this check altogether (highly not recommended, but I guess you should still have an option)

I’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/testing can be enabled without sacrificing the general protection of this rule?

Not easy to name the option but maybe something like:

{
  "allowedExceptionsToLazyLoadingCheck": [
    "@foo/bar/testing",
    "@baz/qux/testing"
  ]
}

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-line overrides the check on the graph generation level (influencing how the graph deps are constructed) while // eslint-disable-next-line @nrwl/nx/enforce-module-boundaries only 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-boundaries as workaround but we prefer to wait for a fix. Thanks!