Skipping npm clean-install if node_modules folder exist for storefront/admin build scripts?

[AndreasA]

Please describe the feature you would like to see implemented.

As the required node_modules for administration/storefront should actually stay the same until a Shopware update for that dependency, which will install administration/storefront without the node_modules folder anyway, wouldn't it make sense to only do the clean-install if the node_modules folder is missing?

That would speed up local builds after the first build, avoid unnecessary IDE reindexing and also be in sync with how the node_modules are installed for plugins.

Maybe a parameter/env variable could be used to force clean-install which should then also include the plugins.

[shyim]

@seggewiss can we just do there npm install without clean?

[seggewiss]

@seggewiss can we just do there npm install without clean?

@shyim In my opinion we could do a npm install.

[AndreasA]

I think npm install would also work for plugins. it should be fine as long as a package-lock is provided.

[AndreasA]

@shyim @seggewiss Shall I create a PR to use install in build-administration / build-storefront for plugins and storefront / administration instead of npm clean-install? In which case I would also remove the -d node_modules check for plugins as npm install shouldn't be a real performance issue to just run it.

Or should I create a PR to add the directory exists check for storefront/administration dependencies as well.

[AndreasA]

@shyim @seggewiss I have done some testing and the fastest solution by far is to not do any npm install or clean-isntall, if it isn't necessary.

So independently from using install or clean-install, the installation command should be completely skipped, if the node_modules folder exists.

Maybe add an environment variable to forcefully reinstall - which should be for plugins and storefront - or add one to skip the installation (which should also be valid for plugins and storefront).

[mzeis]

As far as I see, npm install should be fine for Resources/app/storefront/package.json because all packages have exact versions defined. For Resources/app/administration/package.json or plugins, this might give unwanted results because npm install will update package-lock.json if non-exact versions are defined (using ~ or ^). If the administration package.json also only uses exact versions, we could use npm install at least for the core package.json files.

I wouldn't omit the install command if the node_modules folder exists. Just because it's there it doesn't mean that the package versions are installed which have been defined in package-lock.json.

[AndreasA]

@mzeis From what I know, could be wrong though as npm is not really my expertise:

1. npm install does take the package-lock.json into account.
   • It might be updated in case URLs or similar changes occur regarding packages but no version updates occur.
     • As long as the version in the package-lock.json is inside the semver range provided by the package.json.
     • And the package is in the package-lock.json.
     • Which has to be the case if both files are kept up-to-date.
     • The only scenario where they might differ is, if the package.json was updated but not the package-lock.json. However, that case is an issue in itself as nobody actually knows anymore if the package-lock.json is correct or the package.json.
     • However, npm ci would fail to install in such a scenario, so it could be used to verify that the package-lock.json is up-to-date in Shopware's pipelines.
     • See also: https://docs.npmjs.com/cli/v8/commands/npm-install#description and https://docs.npmjs.com/cli/v8/commands/npm-install#package-lock
   • As npm clean-install removes the node_modules folder it is terrible to use during development, as it means:
     • all packages/dependencies have to be completely reinstalled which is quite slow in comparison to just exchanging some packages due to version changes.
     • Maybe even redownloaded. Though I think npm has a cache for packages similar to composer. However, it could have been cleared or be transient.
     • The IDE needs to reindex all those dependencies as the whole folder was deleted, which is a performance nightmare.
     • However, if the command is skipped completely in case there is a node_modules folder, it wouldn't matter.
2. The node_modules folder does not exist after a version update of the corresponding Shopware package.
   • As composer replaces the whole package/folder and the new version does not contain any node_modules folders.
   • So the required dependencies/packages cannot change until node_modules is gone as only then the version of the corresponding shopware package acutally changed.
   • Skipping npm clean-install / npm install is actually already done for plugins, if you check the scripts, and it works fine there because of this reason. It would already be an issue otherwise.
   • However, for plugins that are currently developed this might actually be an issue as the package.json for those might change during development and then the current script would not install corresponding updates/changes.
     • Although in such a scenario the developer will do the npm install / npm update manually in the corresponding plugin folder. Therefore, that is not an issue either.

[mzeis]

@AndreasA I am not a npm expert, but from what I know, npm install can update packages even when package-lock.json exists if there are no exact version constraints. See https://stackoverflow.com/a/70212123/558524 for one of the recent links on this. We had this problem on one of our Non-Shopware project. It could be though that this happened because of what you explained in detail here, so please take this with a grain of salt.

I agree that npm clean-install is a pain to use during development, especially when it's difficult to use the hot-proxy mode e.g. in a Docker setup, so I would be more than glad to get rid of this. :+1: It's just important that one gets at least a warning if there could be unwanted updates during the process.

[AndreasA]

@mzeis Should hopefully have been only due to some version changes in the package.json, see also: https://dev.to/saurabhdaware/but-what-the-hell-is-package-lock-json-b04

However, to be honest I really think npm is in general weird how they handle the package-lock.json as I personally think install should not change it at all like with composer. It would just make way more sense. But as mentioned as long as package.json and package-lock.json are in sync that is fine. However, if a new package is added to the package.json, then it might be that other packages would be updated with npm install as well.

[amenk]

Are there still concerns to use "install" instead of "clean-install" ? It's such a huge waste of resources (time/network/energy) and we should replace it soon, if there are not problems caused by it. If it could cause problems, we at least could at a a switch for it at least. But of course not everything should be made configurable, if there is no need for it :-)

[shyim]

I adjusted the flex template https://github.com/shopware/recipes/commit/95ae75634abbbafe7db27e97d06a8e44cf75b39a

[amenk]

What about this repo? Will it also be updated? Or is flex the new way to go anyways?

[shyim]

We want to update the old template until 6.5 and not break and really add anything anymore. With the flex setup we have a new green ground can do things a complete different