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

npm install is never run when dependencies are added by others #8182

Closed
Artur- opened this issue Apr 27, 2020 · 4 comments · Fixed by #8282
Closed

npm install is never run when dependencies are added by others #8182

Artur- opened this issue Apr 27, 2020 · 4 comments · Fixed by #8282

Comments

@Artur-
Copy link
Member

Artur- commented Apr 27, 2020

When another project member adds a dependency, my project is broken until I remove node_modules.

To reproduce:

vaadin init hello
cd hello
mvn install -Pproduction # Runs npm install
git init && git add -A &&  git commit -a -m "Initial" # To know what is happening

# Now we simulate the other user who installs a-avataaar
npm i a-avataaar --save
mvn install -Pproduction # Runs npm install because hash does not match
ls node_modules/a-avataaar/ # Yup, it is there
git commit -m "Add dependency" -a # Make the commit

# Now go back and pretend to be the original user
git revert head
rm -rf node_modules/ && mvn install -Pproduction # Go back to the previous state, runs npm install
ls node_modules/a-avataaar/ # Nope, not there. Should not be either
git reset head^ --hard # Apply the patch from the other user
mvn install -Pproduction # SHOULD run npm install but does not
ls node_modules/a-avataaar/ # Nope, not there. SHOULD BE THERE
@web-padawan
Copy link
Member

Originally reproduced in Starter Wizard, when added new lines to "devDependencies" manually.

@Artur-
Copy link
Member Author

Artur- commented Apr 28, 2020

Would a potential solution to this be to store the hash inside node_modules, e.g. node_modules/.vaadin with {hash: 'abc123'} so that it would represent what was last installed. If the hash in package.json does not match, then npm install must be run.

This would also solve the "npm install was interrupted" problem as the hash should only be written once npm install has finished successfully.

@caalador
Copy link
Contributor

That sounds like a good suggestion.

@Artur-
Copy link
Member Author

Artur- commented Apr 28, 2020

Presumably if both the package.json and node_modules hash was updated by a postinstall script in the project's package.json then you would also avoid running npm install again when starting the server with mvn after installing a package using npm.

Right now it goes like this in 14.2+

vaadin init hello
cd hello
mvn install -Pproduction # Runs npm install like it should
npm i --save a-avataaar # Installs but does not update anything
mvn install -Pproduction # Runs npm install because hash was not updated

@caalador caalador self-assigned this May 8, 2020
@caalador caalador moved this from Product backlog to In progress in OLD Vaadin Flow ongoing work (Vaadin 10+) May 8, 2020
caalador added a commit that referenced this issue May 8, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
caalador added a commit that referenced this issue May 8, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
caalador added a commit that referenced this issue May 11, 2020
Generate a full hash to mainPackageJson
and store it after npm install to
node_modules/.vaadin

Fixes #8182
caalador added a commit that referenced this issue May 11, 2020
Generate a full hash to mainPackageJson
and store it after npm install to
node_modules/.vaadin

Fixes #8182
caalador added a commit that referenced this issue May 11, 2020
Generate a full hash to mainPackageJson
and store it after npm install to
node_modules/.vaadin

Fixes #8182
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from In progress to Done - pending release May 11, 2020
OLD Vaadin Flow bugs & maintenance (Vaadin 10+) automation moved this from P1 - High priority to Closed May 11, 2020
caalador added a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
caalador added a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182

# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
caalador added a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182

# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
caalador added a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
caalador added a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
caalador added a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
joheriks pushed a commit that referenced this issue May 11, 2020
Generate a full hash to mainPackageJson
and store it after npm install to
node_modules/.vaadin

Fixes #8182
pleku pushed a commit that referenced this issue May 11, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182

# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
mehdi-vaadin pushed a commit that referenced this issue May 12, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182

# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
pleku pushed a commit that referenced this issue May 14, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182

# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
pleku pushed a commit that referenced this issue May 14, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182

# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
pleku pushed a commit that referenced this issue May 20, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
joheriks pushed a commit that referenced this issue Jun 4, 2020
To notice outside changes to package.json
better we add the hash to node_modules
so we can cross check the value.

Fixes #8182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants