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 NPM 8, 9, 10 and PNPM 8, remove support for NPM 6 #235

Merged

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Feb 15, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Fixed tickets fixes #88
Related issues/PRs #issuenum
License MIT
Documentation PR sulu/sulu-docs#prnum

What's in this PR?

Add support for NPM 8, 9, 10 and PNPM 8, remove support for NPM 6

Why?

Node 16 is going for end of life: https://nodejs.org/en/about/previous-releases

Example Usage

rm -rf ./**/node_modules/
rm -rf ./**/package-lock.json

cd assets/admin
npm -g i npm@10

npm install
npm run build
rm -rf ./**/node_modules/
rm -rf ./**/package-lock.json
rm -rf ./**/pnpm-lock.yaml

cd assets/admin
curl -fsSL https://get.pnpm.io/install.sh | sh -

pnpm install
pnpm run build

BC Breaks/Deprecations

No support for NPM 6 / symlinked local packages.

Disadvantages

No watch task over vendor package developments as install requires install-links=true which ends not longer uses symlinks. Instead a tarball is created and install, but it installs correctly dependencies of local dependencies. Which does not work for file: packages.

Why this opens a wider range for support of modern npm and pnpm versions. Yarn is still a problem and not supported by sulu. This includes not only yarn 1 but also 2,3,4.

Comment on lines +17 to +24
# unfortunataly, npm >= 7 ignores dependencies of local dependencies to handle them the same way as normal
# dependencies we require to use install-links which will create a tar ball for all packages and install
# it like a normal dependency with its dependencies
install-links=true

# pnpm does not install dependencies of local dependencies correctly by default but if use the shamefully-hoist
# configuration it also installs them correctly and can so be correctly build via pnpm
shamefully-hoist=true
Copy link
Member Author

Choose a reason for hiding this comment

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

This are the main changes required for newer NPM version and for using PNPM

@alexander-schranz alexander-schranz marked this pull request as draft February 15, 2024 18:42
@alexander-schranz alexander-schranz force-pushed the enhancement/node-npm-upgrade branch 3 times, most recently from bdec5d9 to f908c40 Compare February 15, 2024 18:46
@chirimoya
Copy link
Member

@alexander-schranz how will development work change with the missing watch task? Always reinstall and build? Can that be done with some kind of automation?

@alexander-schranz
Copy link
Member Author

alexander-schranz commented Feb 16, 2024

@chirimoya

how will development work change with the missing watch task? Always reinstall and build? Can that be done with some kind of automation?

Developing in core would be easier for most when just work in the sulu/sulu itself not longer over the sulu/skeleton.

Developing of third party bundles is more an issue, normally npm link would solve it but it ignores the install-links flag which make it not usable for sulu. Still manually symlinks would allow still have the same behaviour as today, aslong as there are no conflicting dependencies:

npm install # install all dependencies

rm -rf node_modules/sulu-admin-bundle
ln -s @sulu/vendor/sulu/sulu/src/Sulu/Bundle/AdminBundle/Resources/js node_modules/sulu-admin-bundle

We could provide a script doing this mechanism something like npm run prepare-dev-links.

@alexander-schranz alexander-schranz marked this pull request as ready for review March 15, 2024 08:49
mysql-version: '8.0'
php-extensions: 'ctype, iconv, intl, mysql, pdo_mysql, php_fileinfo, imagick'
php-extensions: 'ctype, iconv, intl, mysql, pdo_mysql, php_fileinfo, gd, sodium'
Copy link
Member Author

Choose a reason for hiding this comment

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

Bildschirmfoto 2024-03-15 um 10 31 41

Looks like Imagick on PHP 8.3 is not yet supported for windows.

The sodium PHP Extension seems not longer be part of setup-php of winodws. So we need to enable it.

@alexander-schranz alexander-schranz enabled auto-merge (squash) March 15, 2024 09:49
@alexander-schranz alexander-schranz merged commit 7cd92e2 into sulu:2.6 Mar 15, 2024
4 checks passed
@alexander-schranz alexander-schranz deleted the enhancement/node-npm-upgrade branch March 15, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants