Do not delete generated package.json on mvn clean

[Artur-]

For various reasons, you run mvn clean package on your project instead of mvn package. This might be to update dependencies and remove old ones, to make sure code is re-generated or whatever other reason.

When doing mvn clean package, npm install is triggered even though the generated package.json has not been updated. Because the generated file is stored in target, it is removed by mvn clean and comparing the newly generated one to the existing one does not work.

This should be solved so mvn clean package does not need to run npm install.

Potential solutions:

1. Move the generated package.json out of target so it is not removed. E.g. use the same webpack.config.js+webpack.generated.js pattern and name it /package.generated.json
2. Store a hash of the generated package.json contents somewhere else for comparison purposes, e.g. main package.json

[mvysny]

I agree, the npm install should be run only when the package.generated.json changes. We could also store the hash into the node_modules folder (given it's not deleted by mvn clean).

There are couple of reasons to avoid npm install:

1. Decrease build time
2. Re-generating 10000+ files kills Intellij file watcher (or at least used to kill, with 2019.1.3)
3. Anti-virus software may slow things to a crawl if they decide to scan every of 10000+ files.
4. Server-side guys will be annoyed that they're forced to wait until "that JavaScript thing" finally builds. Many of projects factored out GWT compilation to a standalone jar artifact for precisely this reason.

[Legioth]

I don't like the first suggested solution of putting the generated package.json file in the root of the project since it would then be included in version control unless we also update .gitignore in all starters and such. I don't think anything will go wrong if it's in version control, but it will confuse users when their version control system reports that they have changes in files that they haven't touched.

Another way of fixing this would be to update all starters to exclude that particular file from mvn clean. One potential drawback is that it might be confusing if you still have a /target directory after mvn clean although it's almost empty.

Storing a copy of the file or a hash of the contents in node_modules is maybe not academically correct, but it might actually work.

Storing a hash as a "version number" in the entry in the main package.json file might also work. Giving a proper version number to the generated package.json might also remove the need for forcibly removing package-lock.json, which also contributes to making npm install take longer.

[retomerz]

First, thanks for investing this "speed issue". I am the one which reported this (along with #6150) via vaadin.com support portal.

I don't like the first suggested solution of putting the generated package.json file in the root of the project since it would then be included in version control unless we also update .gitignore in all starters and such. I don't think anything will go wrong if it's in version control, but it will confuse users when their version control system reports that they have changes in files that they haven't touched.

Another way of fixing this would be to update all starters to exclude that particular file from mvn clean. One potential drawback is that it might be confusing if you still have a /target directory after mvn clean although it's almost empty.

Storing a copy of the file or a hash of the contents in node_modules is maybe not academically correct, but it might actually work.

Storing a hash as a "version number" in the entry in the main package.json file might also work. Giving a proper version number to the generated package.json might also remove the need for forcibly removing package-lock.json, which also contributes to making npm install take longer.

Please correct me, but at the moment there is already a generated file webpack.generated.js in the project root which should not committed to the VCS according to your documentation [1] (Point 6).
But basically I agree with you, the project root should not be spammed with generated/temporary files.
They should be placed in /target. I am also not a fan of the solution to exclude the /target/package.json from the Maven clean plugin.
I think "mvn clean" should delete the whole /target directory.

I know npm and the vaadin-maven-plugin too little, but /target/package.json looks for me that the file can be committed to the VCS.
So I would also suggest to introduce one more file on the project root, for example package.flat.json.
/target/package.json is compared with package.flat.json and will be overwritten only when its necessary (dependency changed).

Just my 2 cents.

[1] https://vaadin.com/docs/v14/flow/v14-migration/v14-migration-guide.html

[Legioth]

webpack.generated.js is done in an awkward way because of difficulties with putting that file in any other place. In general, we should still try to avoid files that need to be treated in special ways.

While it's technically possible to have the internally generated package.json file in version in the root of the project (with a different name to avoid conflicting with the main package.json file), that would introduce yet another file that you either need to add to .gitignore or commit even though you haven't made any changes to it.

For that reason, we would like to find a solution that can have the file in some other location.

[Artur-]

If you have two package.json files in the same folder then you might have some problems if both happen to install something into the same node_modules folder

[caalador]

As the contents for the folder ./target/frontend will be copied to ./node_modules/@vaadin/flow-deps we could do a compare on build. if we haven't run npm install we won't have the file available in node_modules as it is copied at that time, and if we have run it we can compare the created package.json against the node_modules version and only if they differ we execute npm install.

[Legioth]

the contents for the folder ./target/frontend will be copied to ./node_modules/@vaadin/flow-deps

On my machine, the contents aren't copied but instead, flow-deps is a symlink to ../../target/frontend.

[caalador]

On my machine, the contents aren't copied but instead, flow-deps is a symlink to ../../target/frontend.

Right. Also on windows its apparently a JUNCTION and not an actual copy.

[caalador]

Acceptance criteria:

• Store the hash of the generated package.json as the main package.json version when npm install has run.
• Check for the need to run npm install by checking the hash of the generated package.json against the version in the main package.json

[Legioth]

As a workaround, you can configure maven-clean-plugin to not remove the file target/frontend/package.json.

[Legioth]

The merged patch is not enough to fix this issue because the ordering inside target/frontend/package.json changes from run to run. This also causes the hash to change.

[denis-anisimov]

That means that acceptance criteria is not correct.

[Legioth]

This is still not resolved properly, even though it's slightly less obvious now. I don't know exactly what goes on, but doing mvn clean jetty:run will not honour what's in package-lock.json but instead install the newest available version from a dependency range. To reproduce this, we need to first do some setup to emulate a situation where an (old) version is kept in use only because of package-lock.json:

1. Update Beverage Buddy to use 2.1-SNAPSHOT of flow-server
2. Run mvn clean jetty:run once to ensure vaadinAppPackageHash is updated in package.json.
3. Add "left-pad": "1.2.0" as a dependency in package.json.
4. Do npm install and then npm ls left-pad to verify that version 1.2.0 is indeed installed
5. Update package.json to instead use "left-pad": "^1.2.0", run npm install again and verify that the same version is still used.
6. Make a copy of your package-lock.json file for future reference
7. Observe that after running npm install after removing node_modules, then you will still have version 1.2.0 of left-pad. This means that the project has reproducible builds as long as package-lock.json is in version control.
8. Observe that after running npm install after removing both node_modules and package-lock.json, then you will get version 1.3.0 of left-pad. Furthermore, the contents of package-lock.json will be different.
9. Undo the "destructive" side effects of the previous step by putting back the package-lock.json file from step 6., removing node_modules and doing npm install again (and verify that version 1.2.0 is used and that package-lock.json still has the original contents from step 6).

After this setup, the situation is thus emulating what would have happened if version 1.3.0 would have been recently released.

In this situation, doing mvn jetty:run will keep using version 1.2.0 as expected. The problem is that you will end up with version 1.3.0 and a modified package-lock.json after mvn clean jetty:run.

[Legioth]

In fact, the only thing that is relevant for this ticket is the very last detail that npm install still seems to happen when doing mvn clean jetty:run. The fact that npm install causes unexpected updates to happen is the topic of a separate ticket.

EDIT: Separate issue created for preserving the installed left-pad version even in cases when npm install should actually be run: #6430

[caalador]

The current problem is that we do clean up because we see that the shrinkwrap version has changed.
This problem is probably the cause also for 6430 where package-lock.json is removed and updated.

[denis-anisimov]

shrinkwrap version should be changed only when the platform release differs from the one is written in some package* files.
That may happen one time and we should keep this behavior since otherwise we will have again version conflicts issue.
But that should happen once: after the first time the versions should be the same and files should not be removed.

[caalador]

The problem is that when we clean the version is lost and when we check in package-lock.json we look for value instead of version so we never find a version to check for.

[denis-anisimov]

So it was just a typo, my bad.

[Legioth]

Great. Now it seems to work even in my sneaky case. Let's just remember that there are (at least) three different patches that need to be cherry picked for this issue.

[denis-anisimov]

Let's just remember that there are (at least) three different patches that need to be cherry picked for this issue.

Yes, that's the trickiest thing.