build-frontend/npm fails to update the package-lock.json completely (when updating Vaadin)

AB-xdev commented 3 years ago

Description of the bug

Backstory:

We recently switched from Vaadin 14.4.3 to 14.4.8 in https://github.com/xdev-software/vaadin-date-range-picker/pull/63. We expected that this also closes the following security alert: https://github.com/xdev-software/vaadin-date-range-picker/pull/55

Dependabot created a PR, where it updated the Vaadin-Version in the pom.xml, however we still have to update the package.json and package-lock.json manually, because dependabot can't do that. So I checked out the branch and executed mvn clean install -Pproduction. This updated the package.json. But it looks like it failed to update the package-lock.json completely.

We only noticed this weird circumstance because the security warning was still there, beside being on the (then) latest Vaadin version.

The problem was solved by deleting node_modules, package.json, package-lock.json, webpack.config.js locally and rerunning mvn clean install -Pproduction: https://github.com/xdev-software/vaadin-date-range-picker/commit/89c3b86484cf8186a53ff37c3c42d0f5d202ddf6

Minimal reproducible example

Checkout https://github.com/xdev-software/vaadin-date-range-picker
Change to the state before the vaadin update occured: git checkout 3a7660c
Run mvn clean install -Pproduction → This should generate the node_modules
Now go to the commit in which dependabot updated the pom.xml to 14.4.8 git checkout 0cc7df6
Run mvn clean install -Pproduction again. As you see, the package.json and package-lock.json get updated. However package-lock.json's "ini" dependency should now be on version 1.3.8, but it's just 1.3.5
But if you delete
- package.json
- package-lock.json
- node_modules
- webpack.config.js
and rerun mvn clean install -Pproduction the files are different and the package-lock.json is now fully updated as expected.

Expected behavior

The results of mvn clean install -Pproduction in package-lock.json should not differ when either
- having an existing package.json/package-lock.json
- or having none

Suggested workaround

Ignore package.json, package-lock.json and webpack.config.js in git

Versions:

- Vaadin / Flow version: 14.x
- Java version: 11
- OS version: Win 10

Artur- commented 3 years ago

The results of mvn clean install -Pproduction in package-lock.json should not differ when either having an existing package.json/package-lock.json or having none"

I think this expectation is wrong because the whole point of package-lock.json from the point of view of npm is that the versions mentioned in package-lock.json are installed. If you have a package-lock.json that says ini version 1.3.5 should be used, then npm will install that. If you do not have a package-lock.json containing ini, then npm will pick the latest version available that matches requirements of other packages. In this case, ^1.3.5 will be resolved to 1.3.8 and then 1.3.8 will be entered in package-lock.json to ensure that the next npm install will install that version.

AB-xdev commented 3 years ago

At first: Thanks for the quick feedback 😄

Okay that ... kind of makes sense. I don't think I'm going to like that behavior but I understand the circumstances.

However isn't there a way of "forcing" updates (beside deleting the package-lock.json) when running build-frontend? So when running "npm install" all versions that are not fixed (e.g. ^1.3.5) get updated to the most recent version (1.3.8).

It doesn't sound good when GitHub recommends security updates to you for npm packages and they are impossible to fix because npm doesn't update them.

Artur- commented 3 years ago

You can run npm update manually and should get the same result as when removing package-lock.json but Flow is not running that automatically

Artur- commented 3 years ago

There is the opposite problem that sometimes new npm packages are broken. If everything was always auto updated then all projects in the world would end up in a broken state immediately when a broken npm package is released.

caalador commented 3 years ago

Basically Vaadin should in NPM mode (but not for pnpm) clean up the package-lock.json and node_modules in the case where the Vaadin version has updated by comparing the shrinkwrap versions. This should be done automatically even for bugfix changes.

vaadin / flow