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

fix: lazily run npm install when applying 'prettier' and client modules #11113

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

murdos
Copy link
Contributor

@murdos murdos commented Oct 13, 2024

If a package-lock.json is detected, and npm command is present, run npm install.

Fixes #11106

If we agree on the solution, we should do the same with any module that touches prettier or eslint configuration:

  • typescript
  • react-core
  • angular-core
  • svelte

If a package-lock.json is detected, and npm command is present, run npm install.

Fixes jhipster#11106
@murdos murdos added the area: bug 🐛 Something isn't working label Oct 13, 2024
@pascalgrimaud
Copy link
Member

Let me having a look on this solution

@pascalgrimaud pascalgrimaud self-assigned this Oct 14, 2024
NpmInstallationType npmInstallationType = npmInstallationReader.get();
switch (npmInstallationType) {
case UNIX:
execute(folder, "npm", "install");
Copy link
Member

Choose a reason for hiding this comment

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

maybe it should be ci instead of install ?

execute(folder, "npm", "install");
break;
case WINDOWS:
execute(folder, "npm.cmd", "install");
Copy link
Member

Choose a reason for hiding this comment

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

maybe it should be ci instead of install ?

Assert.notNull("folder", folder);

if (!folder.fileExists("package-lock.json")) {
log.info("No package-lock.json found, npm install skipped");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.info("No package-lock.json found, npm install skipped");
log.info("No package-lock.json found, npm ci skipped");

execute(folder, "npm.cmd", "install");
break;
case NONE:
log.info("No npm installed, can't install project");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.info("No npm installed, can't install project");
log.info("No npm installed, can't launch npm ci");

Process process = new ProcessBuilder(commands).directory(folderFile(path)).start();

if (failedExecution(process)) {
throw new NpmInstallException("Error during formatting, process failed");
Copy link
Member

Choose a reason for hiding this comment

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

it could be used for another command, so it should not be related to formatting


traceProcess(String.join(" ", commands), process);
} catch (IOException e) {
throw new NpmInstallException("Error during formatting: " + e.getMessage(), e);
Copy link
Member

Choose a reason for hiding this comment

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

it could be used for another command, so it should not be related to formatting

} catch (InterruptedException e) {
Thread.currentThread().interrupt();

throw new NpmInstallException("Error during formatting: " + e.getMessage(), e);
Copy link
Member

Choose a reason for hiding this comment

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

it could be used for another command, so it should not be related to formatting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: bug 🐛 Something isn't working theme: client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to apply typescript and vue-core in a second time
2 participants