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

Add support for pnpm and Yarn PnP #1142

Merged
merged 1 commit into from Aug 30, 2022
Merged

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Aug 2, 2022

For Yarn PnP and pnpm support, Encore needs to configure peerDependenciesMeta as explained in #655.

I've configured every packages listed in lib/features.js as optional peerDependencies, and webpack as a required peer dependency.

I'm not sure of how we can write tests for that, any idea @weaverryan? Maybe an E2E test with a real app/dedicated CI workflow?
EDIT: implemented E2E tests for Yarn PnP and pnpm.

Thanks!

yarn.lock Outdated Show resolved Hide resolved
@Kocal Kocal changed the title feat: configure peerDependenciesMeta, close #655 feat!: configure peerDependenciesMeta, close #655 Aug 2, 2022
@Kocal Kocal force-pushed the fix/GH-655 branch 3 times, most recently from 32fd06f to 05319bf Compare August 2, 2022 17:41
@Kocal Kocal mentioned this pull request Aug 2, 2022
@Kocal Kocal force-pushed the fix/GH-655 branch 4 times, most recently from a5e59bb to 0299a7c Compare August 2, 2022 20:04
@Kocal
Copy link
Contributor Author

Kocal commented Aug 2, 2022

Fixing tests for windows is making me crazy 🤯

@Kocal Kocal changed the title feat!: configure peerDependenciesMeta, close #655 Add support for pnpm and Yarn PnP Aug 11, 2022
@arderyp
Copy link

arderyp commented Aug 18, 2022

Thanks for doing all of this @Kocal, looks like you're almost there!

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is great - thanks for taking this on!

@@ -0,0 +1,48 @@
name: Testing apps
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more descriptive name we could use here?

Copy link
Contributor Author

@Kocal Kocal Aug 24, 2022

Choose a reason for hiding this comment

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

I don't know, this workflow means to test the installation and run of Encore on real projects/app.

Do you have something in mind?

Copy link
Member

Choose a reason for hiding this comment

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

You may have just suggested a good one

name: Testing installation in real apps

"vue": "^3.2.14",
"vue-loader": "^17.0.0",
"vue-template-compiler": "^2.5",
"webpack": "^5.72",
Copy link
Member

Choose a reason for hiding this comment

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

No webpack-cli here?

Copy link
Contributor Author

@Kocal Kocal Aug 24, 2022

Choose a reason for hiding this comment

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

I removed it since webpack already ships webpack-cli as a dev dependency (useful for when developing Encore) and as an optional peer dependency (useful for Encore users).

Copy link
Member

Choose a reason for hiding this comment

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

dev dependencies of webpack have no impact when developing Encore. We are not developing webpack in such situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I have anything to change here? ATM webpack-cli, in this package.json is present in devDependencies only.

"terser-webpack-plugin": "^5.3.0",
"tmp": "^0.2.1",
"webpack": "^5.72",
"webpack-cli": "^4.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

Will this mean that webpack and webpack-cli will now need to live in the user's package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For webpack yes, but for webpack-cli it looks like it depends of what package manager/script runner the user use.

If you take a look to package.json files used in Yarn PNP and pnpm testing apps, you will notice:

  • the Yarn PNP project need to require (as dev dependency) webpack-cli, otherwise Encore won't be able to run (I don't remeber the issue, but it can be easily reproduced)
  • the pnpm project need to not require webpack-cli, otherwise Encore won't be able to run either 🤔 (and I don't remember the issue aswell)

Maybe Yarn PNP and pnpm have a different way to deal with dependencies's peer dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

yes. But this is necessary anyway as soon as you want to install an extra webpack plugin and being sure that it uses the same webpack version than the one used for the build, which is a must-have. If we want to hide the tool, we need to hide it entirely, which is not the purpose of Encore as it is a configurable tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. In our case we need to use the same version of webpack between Encore and Cypress for Component testing, otherwise we got this kind of error:
image

Copy link
Member

Choose a reason for hiding this comment

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

The situation about webpack-cli is still not clear to me:

A) (?) For non-PNP, you will or won't need to install webpack-cli? Which is it?
B) For Yarn PNP, it MUST be installed
C) For pnpm it must NOT be installed

Will the code in Webpack's executable (https://github.com/webpack/webpack/blob/9fcaa243573005d6fdece9a3f8d89a0e8b399613/bin/webpack.js#L90-L118) effectively enforce whether or not the user will need to install it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A) For npm (8), If webpack-cli is installed (or not) alongside webpack, then webpack prompts to install webpack-cli:

➜  npm git:(fix/GH-655) ✗ npm version
{
  npm: '8.3.1',
  node: '16.14.0',
  v8: '9.4.146.24-node.20',
  uv: '1.43.0',
  zlib: '1.2.11',
  brotli: '1.0.9',
  ares: '1.18.1',
  modules: '93',
  nghttp2: '1.45.1',
  napi: '8',
  llhttp: '6.0.4',
  openssl: '1.1.1m+quic',
  cldr: '40.0',
  icu: '70.1',
  tz: '2021a3',
  unicode: '14.0',
  ngtcp2: '0.1.0-DEV',
  nghttp3: '0.1.0-DEV'
}
➜  npm git:(fix/GH-655) ✗ npm run encore dev

> encore
> encore "dev"

Running webpack ...

CLI for webpack must be installed.
  webpack-cli (https://github.com/webpack/webpack-cli)

We will use "npm" to install the CLI via "npm install -D webpack-cli".
Do you want to install 'webpack-cli' (yes/no): %

And if I type yes, webpack is not able to find webpack-cli/package.json anymore:

➜  npm git:(fix/GH-655) ✗ npm run encore dev

> encore
> encore "dev"

Running webpack ...

CLI for webpack must be installed.
  webpack-cli (https://github.com/webpack/webpack-cli)

We will use "npm" to install the CLI via "npm install -D webpack-cli".
Do you want to install 'webpack-cli' (yes/no): yes
Installing 'webpack-cli' (running 'npm install -D webpack-cli')...

added 40 packages, and audited 120 packages in 709ms

15 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
Error: Cannot find module 'webpack-cli/package.json'
Require stack:
- /Users/halliaume/workspace-os/webpack-encore/node_modules/webpack/bin/webpack.js
- /Users/halliaume/workspace-os/webpack-encore/bin/encore.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.resolve (node:internal/modules/cjs/helpers:108:19)
    at runCli (/Users/halliaume/workspace-os/webpack-encore/node_modules/webpack/bin/webpack.js:65:26)
    at /Users/halliaume/workspace-os/webpack-encore/node_modules/webpack/bin/webpack.js:154:5
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/halliaume/workspace-os/webpack-encore/node_modules/webpack/bin/webpack.js',
    '/Users/halliaume/workspace-os/webpack-encore/bin/encore.js'
  ]
}

B) For Yarn PNP, if webpack-cli is not installed alongside webpack, we have the following error when running Encore :

➜  yarn_pnp git:(fix/GH-655) ✗ yarn encore dev
Running webpack ...

/Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.pnp.cjs:17149
      Error.captureStackTrace(firstError);
            ^

Error: webpack tried to access webpack-cli (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.

Required package: webpack-cli (via "webpack-cli/package.json")
Required by: webpack@virtual:dc3fc578bfa5e06182a4d2be39ede0bc5b74940b1ffe0d70c26892ab140a4699787750fba175dc306292e80b4aa2c8c5f68c2a821e69b2c37e360c0dff36ff66#npm:5.74.0 (via /Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.yarn/__virtual__/webpack-virtual-56df3fe76f/0/cache/webpack-npm-5.74.0-f5b838a00d-320c41369a.zip/node_modules/webpack/bin/)

Ancestor breaking the chain: @nuxt/friendly-errors-webpack-plugin@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:2.5.2
Ancestor breaking the chain: @symfony/webpack-encore@virtual:dc3fc578bfa5e06182a4d2be39ede0bc5b74940b1ffe0d70c26892ab140a4699787750fba175dc306292e80b4aa2c8c5f68c2a821e69b2c37e360c0dff36ff66#portal:../..::locator=root-workspace-0b6124%40workspace%3A.
Ancestor breaking the chain: assets-webpack-plugin@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:7.0.0
Ancestor breaking the chain: babel-loader@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:8.2.5
Ancestor breaking the chain: clean-webpack-plugin@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:4.0.0
Ancestor breaking the chain: css-loader@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:6.7.1
Ancestor breaking the chain: css-minimizer-webpack-plugin@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:4.0.0
Ancestor breaking the chain: mini-css-extract-plugin@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:2.6.1
Ancestor breaking the chain: root-workspace-0b6124@workspace:.
Ancestor breaking the chain: style-loader@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:3.3.1
Ancestor breaking the chain: terser-webpack-plugin@virtual:88b9d4c96fcb105b7912cf41ba60d36a65b77cc85cb7e2f55cdd948cb01c6273c2b3f87cf3b37f43c721a4b2a3c78362f07f9f591b99690320400302ac172c1d#npm:5.3.3
Ancestor breaking the chain: webpack-dev-middleware@virtual:6299a4ab64fb92119dc9f6fc2240d23765f58c7d6b46ef18176fa5a773fddeeadb64458c8e83463926a516c544f2edf4a78bfef3cdbf97ab1a34bef7fa72b0ef#npm:5.3.3


Require stack:
- /Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.yarn/__virtual__/webpack-virtual-56df3fe76f/0/cache/webpack-npm-5.74.0-f5b838a00d-320c41369a.zip/node_modules/webpack/bin/webpack.js
- /Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.yarn/__virtual__/@symfony-webpack-encore-virtual-88b9d4c96f/3/bin/encore.js
    at Function.require$$0.Module._resolveFilename (/Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.pnp.cjs:17149:13)
    at Function.resolve (node:internal/modules/cjs/helpers:108:19)
    at runCli (/Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.yarn/__virtual__/webpack-virtual-56df3fe76f/0/cache/webpack-npm-5.74.0-f5b838a00d-320c41369a.zip/node_modules/webpack/bin/webpack.js:65:26)
    at Object.<anonymous> (/Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.yarn/__virtual__/webpack-virtual-56df3fe76f/0/cache/webpack-npm-5.74.0-f5b838a00d-320c41369a.zip/node_modules/webpack/bin/webpack.js:162:2)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Object.require$$0.Module._extensions..js (/Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.pnp.cjs:17193:33)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.require$$0.Module._load (/Users/halliaume/workspace-os/webpack-encore/test_apps/yarn_pnp/.pnp.cjs:17033:14)
    at Module.require (node:internal/modules/cjs/loader:1005:19)

And when I install webpack-cli, it works:

➜  yarn_pnp git:(fix/GH-655) ✗ yarn add -D webpack-cli
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed in 19s 667ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ resolve-from@npm:5.0.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ shallow-clone@npm:3.0.1 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ webpack-cli@npm:4.10.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ webpack-merge@npm:5.8.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ wildcard@npm:2.0.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental
➤ YN0000: └ Completed
➤ YN0000: Done with warnings in 19s 990ms
➜  yarn_pnp git:(fix/GH-655) ✗ yarn run encore dev
Running webpack ...

 DONE  Compiled successfully in 235ms                                                                                                                         11:59:58

3 files written to public/build
Entrypoint app 5.65 KiB = runtime.js 4.92 KiB app.js 749 bytes
webpack compiled successfully

C) Hum, same A) now, I don't know what changed. 🤔

Copy link
Contributor Author

@Kocal Kocal Aug 26, 2022

Choose a reason for hiding this comment

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

For A) and C), adding webpack-cli as a peer dependency for Encore does not change anything.
Looks like https://github.com/webpack/webpack/blob/9fcaa243573005d6fdece9a3f8d89a0e8b399613/bin/webpack.js#L34-L57 is broken?

Copy link
Member

Choose a reason for hiding this comment

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

webpack defines the peerDependenciesMeta to mark webpack-cli as optional without adding webpack-cli in peerDependencies. In such case, yarn automatically treat is a peer dependency (accepting any version). I suspect that pnpm does not do the same (and that webpack is then not compatible with pnpm for their CLI bin relying on accessing webpack-cli)

Copy link
Contributor Author

@Kocal Kocal Aug 26, 2022

Choose a reason for hiding this comment

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

Okay I think I found the issue, when running encore from test_apps/npm, it will use the webpack dependency from Encore project root and not from test_apps/npm, and this webpack dependency does not have webpack-cli installed.

I will push ASAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 575f5e1 (#1142), with webpack and webpack-cli dependencies installed for npm/pnpm/yarn pnp ✨

cc @weaverryan

test_apps/pnpm/package.json Show resolved Hide resolved
@Kocal Kocal requested review from weaverryan and stof and removed request for weaverryan August 26, 2022 16:41
working_directory: test_apps/npm
script: |
npm install --ci
npm add --save-dev ../../webpack-encore.tgz
Copy link
Contributor Author

@Kocal Kocal Aug 26, 2022

Choose a reason for hiding this comment

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

Installing the webpack-encore.tgz (packed in a previous step before) here instead of having it in the package.json, prevents the package manager to fails if checksums are different, which was the case for npm/pnpm/yarn pnp.

@@ -0,0 +1,18 @@
# Testing app: npm
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is relevant, but I prefer this instead nothing.

"license": "UNLICENSED",
"private": true,
"scripts": {
"encore": "encore"
Copy link
Contributor Author

@Kocal Kocal Aug 26, 2022

Choose a reason for hiding this comment

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

I just exposed encore as a script, otherwise npm (through npm run encore) won't find encore executable.

I did the same for pnpm/yarn pnp for consistency purpose, even if it's not needed.

@Kocal Kocal requested review from weaverryan and stof and removed request for stof and weaverryan August 26, 2022 16:44
@stof
Copy link
Member

stof commented Aug 27, 2022

Note that due to the new required peer dependency, this requires releasing it as a new major version.

@weaverryan
Copy link
Member

Note that due to the new required peer dependency, this requires releasing it as a new major version

I was thinking the same, which is ok. To be clear, the new peer dependency is webpack itself.

So I'm still wondering about webpack-cli. This is NOT currently listed as a peerDependency. However, in all of the "test apps", it IS installed. Why isn't this a required peer dependency?

@stof
Copy link
Member

stof commented Aug 29, 2022

It is an optional peer dependency of webpack, that must be installed to be able to use webpack in the CLI (rather than using it programmatically). And so, in Encore, we need it.

I suggest adding webpack-cli as a mandatory peer dependency of Encore, instead of having the first usage of Encore failing and asking the user to install webpack-cli.

@Kocal
Copy link
Contributor Author

Kocal commented Aug 29, 2022

Re-added webpack-cli as a required peer dependency.

@weaverryan
Copy link
Member

weaverryan commented Aug 30, 2022

Thank you Hugo for taking this on! Can you open a recipe PR - we will need to include webpack and webpack-cli in package.json and bump to v4 of Encore. I'm not going to tag quite yet, because I haven't played with this yet and I'd like to get comfortable and do some local testing first.

@weaverryan weaverryan merged commit 417981c into symfony:main Aug 30, 2022
@Kocal Kocal deleted the fix/GH-655 branch August 31, 2022 05:52
@Kocal
Copy link
Contributor Author

Kocal commented Aug 31, 2022

Thanks Ryan :)
I'm gonna open a recipe PR ASAP.

EDIT: opened at symfony/recipes#1123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants