nrwl / nx

Smart Monorepos · Fast CI
https://nx.dev
MIT License
23.28k stars 2.32k forks source link

@nrwl/web hijacks module resolution resulting in incorrect package versions being bundled #6662

Closed brianmcd closed 2 years ago

brianmcd commented 3 years ago

Current Behavior

Note: upgrading to Webpack 5 triggered this issue for us, but I don't believe it's related to Webpack 5 itself. I think it's due to the upgrade causing packages to be installed, which in turn caused yarn to shuffle things around.

After upgrading to Webpack 5 using the Nx migration, we noticed that our usage of the query-string library in our React app stopped working as expected. After digging in, we realized that an older version of query-string was getting bundled into our project. When looking at the compiled build, we found ../../../node_modules/@nrwl/web/node_modules/query-string/index.js as the file that was getting included when doing import queryString from 'query-string';. On our non-webpack5 branch, we saw /home/my-username/work/my-repo/node_modules/query-string/index.js as the path being loaded.

We initially thought this was a Webpack 5 issue, but we found that the Webpack 5 upgrade just happened to cause yarn to move an old version of query-string into @nrwl/web/node_modules. Our main branch doesn't have query-string in @nrwl/web/node_modules. If I manually move the old query-string there, then I see the same behavior on Webpack 4.

Here's how it's ending up there:

=> Found "query-string@6.14.1"
info Has been hoisted to "query-string"
info This module exists because it's specified in "dependencies".
info Disk size without dependencies: "52KB"
info Disk size with unique dependencies: "120KB"
info Disk size with transitive dependencies: "120KB"
info Number of shared dependencies: 4
=> Found "@nrwl/web#query-string@4.3.4"
info Reasons this module exists
   - "@nrwl#web#mini-css-extract-plugin#normalize-url" depends on it
   - Hoisted from "@nrwl#web#mini-css-extract-plugin#normalize-url#query-string"
info Disk size without dependencies: "24KB"
info Disk size with unique dependencies: "56KB"
info Disk size with transitive dependencies: "56KB"
info Number of shared dependencies: 2

We noticed the issue because the version of query-string in @nrwl/web/node_modules is different than the one we want to use, and it's missing support for a feature that we depended on. This problem should generalize to any modules located in @nrwl/web/node_modules. I think they'll always shadow user-installed modules.

My best guess is that this code is responsible: https://github.com/nrwl/nx/blob/3fb591fbd90d349a3a89ee59fe1682496f32deb7/packages/web/src/utils/config.ts#L86-L88

As a test, I created a custom webpack config that calls config.resolve.modules.reverse(), and I don't see the issue in that case. I don't know if that causes other problems, though.

Expected Behavior

When I install a dependency, the version I install should always be the one that's loaded by my app code.

Steps to Reproduce

I have a private repo that I can grant access to that reproduces the issue. Just let me know who to add to it. Reproducing the issue requires a sufficiently unlucky yarn.lock that ends up with query-string (or another dependency used by the app code) in node_modules/@nrwl/web/node_modules. A freshly generated Nx project won't trigger the error off the bat.

Environment

  Node : 14.16.1
  OS   : linux x64
  yarn : 1.22.5

  nx : Not Found
  @nrwl/angular : Not Found
  @nrwl/cli : 12.6.3
  @nrwl/cypress : 12.6.3
  @nrwl/devkit : 12.6.3
  @nrwl/eslint-plugin-nx : 12.6.3
  @nrwl/express : Not Found
  @nrwl/jest : 12.6.3
  @nrwl/linter : 12.6.3
  @nrwl/nest : 12.6.3
  @nrwl/next : Not Found
  @nrwl/node : 12.6.3
  @nrwl/nx-cloud : Not Found
  @nrwl/react : 12.6.3
  @nrwl/schematics : Not Found
  @nrwl/tao : 12.6.3
  @nrwl/web : 12.6.3
  @nrwl/workspace : 12.6.3
  @nrwl/storybook : 12.6.3
  @nrwl/gatsby : Not Found
  typescript : 4.3.5
KeKs0r commented 3 years ago

I think we ran into the same issue some time ago (on nx version 11.X.X). Our temporary work around is this in package.json.

"query-string-original": "npm:query-string@^6.14.0",

Obviously preventing the issue in the first place, would somewhat be preferred.

brianmcd commented 3 years ago

Thanks for adding the label, @FrozenPandaz. Is there anything I can do to help here?

This is a pretty insidious bug, and it's hard to track down when it happens. While I've run into this with query-string, any dependency or sub-dependency of @nrwl/web could show up in its node_modules folder after a package update or package install. When that occurs, whatever version happens to be in @nrwl/web/node_modules will transparently shadow the version installed by the application. To me, it seems like every package update or install is a ticking time bomb. Aside from introducing bugs, it could introduce security vulnerabilities by shadowing an app package with an older, vulnerable version.

7mllm7 commented 3 years ago

I get the same bug, only with parse5. The workaround that @KeKs0r proposed wouldn't work for me, as it's a dependency that one of my dependencies require, so have no real control over it (unless I fork and publish my own version, etc.).

Is there a previous version where this bug does not exist? This is a real show-stopper TBH 😬.

brianmcd commented 3 years ago

@jaysoo - is this something you might be able to offer some insight on? I believe the issue is coming from this change: https://github.com/nrwl/nx/pull/2105. I'm wondering if it's safe to remove resolve(__dirname, '..', '..', 'node_modules') from the modules array via a custom webpack config, or if that will cause other issues. It seemed to work for me, but given how subtle this issue is, I wasn't confident that I didn't just substitute one bug for another.

jaysoo commented 2 years ago

@brianmcd There was a problem when Angular used to be on core-js@2 while React was core-js@3. The core-js package is imported directly in polifills, which is why we needed to customize the module resolution the way we did.

That said, it isn't needed anymore so we're removing that entry. For Nx 13 we will also make sure workspaces don't need their own core-js installed -- it is already a dependency of @nrwl/web.

brianmcd commented 2 years ago

Awesome! Thanks, @jaysoo!

github-actions[bot] commented 1 year ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.