-
Notifications
You must be signed in to change notification settings - Fork 81
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
Javascript setup is not compatible with npm 7 / 8 / 9 #88
Comments
Fixed by sulu/sulu#5859 and #96 🎉 // the sulu/skeleton uses the assets/admin directory as root directory for the administration interface javascript
// application. this application imports the sulu/sulu code by requiring the bundles from the the vendor directory
// in its package.json.
//
// unfortunately this setup causes various problems in the javascript ecosystem because the assets/admin directory
// is not an ancestor of the vendor/sulu/sulu directory which is used to require the code of bundles. for example,
// npm 7 does not install the dependencies of the bundle packages (https://github.com/npm/cli/issues/2339) and older
// npm versions are not able to dedupe packages that are required by a bundle and in the assets/admin directory.
// because of this, packages that were not deduped might be included in the webpack build multiple times. this
// increases the size of the build and will lead to an error in case of the @ckeditor packages:
// http://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-ckeditor-duplicated-modules
//
// to prevent these problems, this file creates a assets/admin/node_modules/node_modules/@sulu/vendor symlink and we
// use the symlinked vendor directory to require the bundles in the assets/admin/package.json.
// this makes the directory that is used for requiring the bundles a descendant of the assets/admin directory and
// allows npm to correctly dedupe the installed packages. |
Damn seems like it doesn't actually work. Really not sure why but currently tested by me and @chirimoya and the packages of the subpackages where not installed. Not sure why or if there was maybe again a change in npm or node, but with current:
it didn't work for me 😢 😠 😡 . So it seems like I did only fix the ckeditor issue and windows build not the npm 😞 |
Thanks for reopening - thats really sad to hear 😢 |
Did test it yesterday but outside node_modules didn't work for me. Not sure why it did work sometime 🙈 |
Okay thats a pity - I guess we need to hope that npm/cli#2339 gets resolved soon then 😕 |
For me it does seem to work if the symlink is outside node_modules, but inside the admin dir. However if I symlink For npm 7 it would also work to just add the list of sulu module directories as an array to the |
One issue I'm still having with this setup is that if the node_modules directory in the root exists, the symlinked package will resolve modules from this directory. The build will succeed but the code won't actually work (wrong version of path-to-regexp causes errors for example). If it doesn't exists everything works fine. |
Hey,
We tried this initially, but we decided against a symlink outside of the
This is really good to know, thanks! Unfortunately this setup will not work with npm 6, right? I think I would prefer such a setup as soon as there is a Node.js LTS release that includes npm 7 per default.
Do you mean the root of the project? Who creates this |
I see!
Yes
Yes, this is the node_modules used for the website itself (webpack/encore).
It is the module resolution that node use, there doesn't seem to be much to influence that. I can't see any workarounds either other than building the admin separately (or with docker for example). |
I think you are right 😕 |
Sadly custom build still don't work under npm 7/8. The related issue npm/cli#2339 still exists. |
@alexander-schranz Will NPM actually fix this because I am not that sure. Maybe you should implement a separate solution instead after all the issue is over a year old and it makes development more tricky due to admin/website might require different NPM versions |
@AndreasA there is already in discussion to use module federation or ddl builds so every bundle provides build version of the code itself and not a project need to compile also the code of the bundles. See also sulu/sulu#6343 (comment) To test that out we first need upgrade to webpack 5. Currently ckeditor was the blocking point there. They fixed that with the latest release 32.0.0 release. At current state if you need to switch npm versions you maybe want to create |
There was an update on the linked issue: npm/cli#2339 (comment) So it is now possible to install the dependencies with NPM 6 requires that we include our dependencies over a local child:
NPM 8.8 seems not to like that and requires an include over:
One change which effects our development process NPM 6 did install the dependencies as symlinks and make so development easier. NPM 8.8 will pack the dependency and install a copy of it, which will make for us the sulu development harder. Still it seems the package installation work. Node 18 or 17 seems not be compatible with webpack 4, both errors in: node:internal/crypto/hash:67 this[kHandle] = new _Hash(algorithm, xofLen);
Node 16 (current LTS) with npm 8.8 seems to work. Solutions for Node 17 / 18 problems seems to update to webpack 5 or use the workaround here: webpack/webpack#13572 (comment) which I tested and did work when adding that lines to sulu webpack config. The major change here is that it will change the hash algorithm and so generate a completely new files also for all static content (fonts, images, ...) which did not change. |
I tried NPM 9 where the
This issue still exist but, there seems currently workaround for this when doing: export NODE_OPTIONS=--openssl-legacy-provider Still npm 8.8 and 9 does not create symlinks which make developing sulu itself only possible inside the So best solution still would be update to webpack 5 and introducing the webpack 5 module federations. Which would also remove the whole dependencies symlink issues. |
I forgot to add I also tried https://github.com/mweststrate/relative-deps package from mweststrate the creator of mobx. Sadly this does also not work as it seems require yarn, versions in package + that the package is published on npm registry. Which make it not usable for us to work with local dependencies. |
Workaround: |
Any news on this? NodeJS 16 support will end in September 2023. I've tried this with v16.19.1 (which comes with npm v8.19.3 by default) and it does not work. Needed to downgrade npm. |
No currently we have not any news here, downgrade or using docker for projects which overwrites something is still required: https://docs.sulu.io/en/2.5/cookbook/build-admin-frontend.html#solution-2-build-manually-with-docker |
Any timeline when this will be updated? With Node 14 I still have to use the old install script from |
Requirement is npm@6 on which node version you run NPM 6 is up to you. I personally use still 14 but our CI Node 16. There is not a timeline yet because the npm issue still not resolved. |
I did test a little bit with But when run: bun run preinstall
bun install
bun run build If bun will adopt in future some of the npm dependencies behaviour I'm not sure but atleast currently from verison |
Bun 1.0.15
|
@simontol The build with |
Hey, it is another problem, but closely related: Using Node > v16 leads to errors:
The solution is to downgrade node to v16 or just set the |
@spackmat Thx for the hint. I also had that openssl issue in the past in older PHP applications. So I patched locally my openssl config this way: https://stackoverflow.com/questions/73832854/php-openssl-pkcs12-read-error0308010cdigital-envelope-routinesunsupported/73858615#73858615. I think the problem should not appear when using docker like documented: https://docs.sulu.io/en/2.5/cookbook/build-admin-frontend.html#solution-2-build-manually-with-docker |
If somebody want to help move things forward here. Please give the changes of #235 a try. |
With the merge of #235 and sulu/sulu#7286 we can finally close this issue. Support for newer versions of npm and support of pnpm and bun will be part of Sulu 2.6. Yarn (all versions yeah they really already have 4) are not supported because how they handle local dependencies. Here the current full list which are supported by the admin build:
This only effects the admin build, for the website / your own CSS build you are still free to use which version and package manager you want to use. |
The javascript setup in the
assets/admin
folder uses thefile:...
syntax to require Sulu packages such as thesulu-admin-bundle
:skeleton/assets/admin/package.json
Lines 15 to 28 in c9640ee
Dependencies that are required like this will be symlinked into the
assets/admin/node_moules
directory. Unfortunately,npm v7
does not install the dependencies of these dependencies at the moment: npm/cli#2339Because of this, the dependencies of the Sulu bundles are not installed and the build fails with a lot of error messages like this:
The text was updated successfully, but these errors were encountered: