-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Conversation
32fd06f
to
05319bf
Compare
a5e59bb
to
0299a7c
Compare
Fixing tests for windows is making me crazy 🤯 |
Thanks for doing all of this @Kocal, looks like you're almost there! |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No webpack-cli
here?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
working_directory: test_apps/npm | ||
script: | | ||
npm install --ci | ||
npm add --save-dev ../../webpack-encore.tgz |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
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 So I'm still wondering about |
It is an optional peer dependency of I suggest adding |
Re-added |
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. |
Thanks Ryan :) EDIT: opened at symfony/recipes#1123 |
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, andwebpack
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!