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(core): resolve webpack loaders with require.resolve() #3436

Merged
merged 2 commits into from
Aug 8, 2020

Conversation

elliottsj
Copy link
Contributor

Same as #3341

With strict package managers such as pnpm or Yarn PnP, transitive dependencies are not hoisted to the root node_modules folder. This means that a webpack config defined within a package like '@nrwl/cypress' cannot resolve loaders like 'ts-loader', unless 'ts-loader' is declared in the workspace's own package.json.

This is a problem because the workspace might define a different version of 'ts-loader', incompatible with the version declared by '@nrwl/cypress/package.json'. The workspace should not need to declare a dependency on 'ts-loader' anyway.

By using require.resolve(), Node.js's module resolution is used instead of webpack's, so this issue is avoided.

See also:

Current Behavior

Workspaces using pnpm must declare dependencies on loaders used by @nrwl/* packages, and manually keep the versions in sync. Or, a public-hoist-pattern must be defined to hoist webpack loaders.

Expected Behavior

pnpm users should not have to declare transitive dependencies or customize package hoisting.

With strict package managers such as pnpm or Yarn PnP, transitive
dependencies are *not* hoisted to the root node_modules folder. This
means that a webpack config defined within a package like
'@nrwl/cypress' cannot resolve loaders like 'ts-loader', unless
'ts-loader' is declared in the workspace's own package.json.

This is a problem because the workspace might define a different version
of 'ts-loader', incompatible with the version declared by
'@nrwl/cypress/package.json'. The workspace should not need to declare
a dependency on 'ts-loader' anyway.

See also:
* pnpm/pnpm#801
* webpack/webpack#5087
@elliottsj
Copy link
Contributor Author

@jaysoo I've published these changes to my local registry following https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md?rgh-link-date=2020-07-20T18%3A06%3A53Z#publishing-to-a-local-registry, and created an app using @nrwl/react. Seems to serve and build without error.

@jaysoo
Copy link
Member

jaysoo commented Aug 5, 2020

Thanks, I'm going to take a look this this week.

@jaysoo jaysoo self-requested a review August 5, 2020 03:00
@jaysoo
Copy link
Member

jaysoo commented Aug 7, 2020

@elliottsj looks like the same issue of apps not being styled is happening. I'm not sure why, but if you can investigate that'd be super helpful. Otherwise I may have time next week to take a look.

Once you have it published to your local registry, run this:

npx create-nx-workspace@latest styling-demo --preset=react --appName=demo --style=css --no-nx-cloud

Let it finish, and then run nx serve demo from the new workspace. When you open the page it'll be unstyled.

Screen Shot 2020-08-07 at 2 12 34 PM

When replacing the 'raw-loader' rule in the `getStylesPartial` function,
check for the absolute path of 'raw-loader' rather than just the name.
@elliottsj
Copy link
Contributor Author

Found the issue; pushed a commit with the fix. Styling works again:

Screen Shot 2020-08-07 at 3 06 14 PM

@jaysoo
Copy link
Member

jaysoo commented Aug 7, 2020

Awesome. Just waiting for the CI to finish.

@jaysoo jaysoo merged commit 5b6df63 into nrwl:master Aug 8, 2020
@elliottsj elliottsj deleted the resolve-webpack-loaders branch August 19, 2020 16:15
Doginal pushed a commit to Doginal/nx that referenced this pull request Nov 25, 2020
* fix(core): resolve webpack loaders with `require.resolve()`

With strict package managers such as pnpm or Yarn PnP, transitive
dependencies are *not* hoisted to the root node_modules folder. This
means that a webpack config defined within a package like
'@nrwl/cypress' cannot resolve loaders like 'ts-loader', unless
'ts-loader' is declared in the workspace's own package.json.

This is a problem because the workspace might define a different version
of 'ts-loader', incompatible with the version declared by
'@nrwl/cypress/package.json'. The workspace should not need to declare
a dependency on 'ts-loader' anyway.

See also:
* pnpm/pnpm#801
* webpack/webpack#5087

* fix(core): resolve absolute 'raw-loader' path

When replacing the 'raw-loader' rule in the `getStylesPartial` function,
check for the absolute path of 'raw-loader' rather than just the name.
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants