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(webpack): ensure loaders are first searched from @nuxt/webpack #7787

Merged
merged 4 commits into from
Jul 27, 2020
Merged

fix(webpack): ensure loaders are first searched from @nuxt/webpack #7787

merged 4 commits into from
Jul 27, 2020

Conversation

matthieusieben
Copy link

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

When nuxt is used as part of a mono repo, NPM & Yarn may install @nuxt/webpack's dependencies in @nuxt/webpack/node_modules instead of the project's root folder. When that is the case, Webpack is not able to find these loaders.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @matthieusieben. It would be nice if you can provide a reproduction for bug-fixes/reports too so we can ensure this fix works

packages/webpack/src/config/base.js Outdated Show resolved Hide resolved
@matthieusieben
Copy link
Author

matthieusieben commented Jul 27, 2020

Reproduce like so:

- test/package.json
{
  "name": "test",
  "private": true,
  "license": "MIT",
  "workspaces": ["packages/*"],
  "dependencies": {
    "css-loader": "^4.0.0"
  }
}

- test/packages/a/package.json
{
  "name": "a",
  "version": "1.0.0",
  "license": "MIT",
  "dependencies": {
    "nuxt": "^2.13.3"
  }
}

Run yarn install in the root dir

 find . -name css-loader -type d
./node_modules/css-loader // v4.x will break builds because Nuxt expects v3.x
./node_modules/@nuxt/webpack/node_modules/css-loader

The "right" css-loader is the one in node_modules/@nuxt/webpack/node_modules, which is also why the module should be before modulesDir.

@matthieusieben matthieusieben requested a review from pi0 July 27, 2020 13:20
@pi0 pi0 changed the title fix(webpack): Ensure all loaders can be found fix(webpack): ensure all loaders can be found Jul 27, 2020
@pi0
Copy link
Member

pi0 commented Jul 27, 2020

@matthieusieben I made a reproduction accordingly: https://github.com/pi0/nuxt-7787

Issue is reproducible but this change won't fix it! We may have to resolve 'css-loader' like this:

    const cssLoader = { loader: require.resolve('css-loader'), options };

@matthieusieben
Copy link
Author

@matthieusieben I made a reproduction accordingly: https://github.com/pi0/nuxt-7787

Issue is reproducible but this change won't fix it! We may have to resolve 'css-loader' like this:

    const cssLoader = { loader: require.resolve('css-loader'), options };

This was my first approach but there are two issues with it:

  • It break backward compatibility of code doing: urlLoader = loader.use.find(use => use.loader === 'url-loader')
  • Not all the loaders referenced in the code are present in the package.json file.

@pi0
Copy link
Member

pi0 commented Jul 27, 2020

@matthieusieben With 93753d3 i can confirm it was fixing issue :)

@pi0 pi0 changed the title fix(webpack): ensure all loaders can be found fix(webpack): ensure loaders are first searched from @nuxt/webpack Jul 27, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2020

Codecov Report

Merging #7787 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #7787   +/-   ##
=======================================
  Coverage   68.88%   68.88%           
=======================================
  Files          90       90           
  Lines        3834     3834           
  Branches     1037     1037           
=======================================
  Hits         2641     2641           
  Misses        969      969           
  Partials      224      224           
Flag Coverage Δ
#unittests 68.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/webpack/src/config/base.js 64.41% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b4aa2...139de10. Read the comment docs.

@matthieusieben
Copy link
Author

So probably you should avoid specify css-loader in monorepo dependencies (but use it for any package that needs it)

Have a look at https://github.com/matthieusieben/nuxt-7787

$ find . -name url-loader -type d
./node_modules/url-loader
./node_modules/@nuxt/webpack/node_modules/url-loader
$ find . -name css-loader -type d
./node_modules/css-loader
./packages/ext/node_modules/css-loader

@matthieusieben
Copy link
Author

matthieusieben commented Jul 27, 2020

@matthieusieben With 93753d3 i can confirm it was fixing issue :)

For me too.

@pi0
Copy link
Member

pi0 commented Jul 27, 2020

Failing windows CI is not related to this PR

@pi0 pi0 merged commit aebf0ff into nuxt:dev Jul 27, 2020
@pi0 pi0 mentioned this pull request Jul 27, 2020
@matthieusieben matthieusieben deleted the patch-1 branch July 27, 2020 18:26
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants