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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,22 @@
import org.springframework.stereotype.Service;
import tech.jhipster.lite.generator.client.vue.core.domain.VueModulesFactory;
import tech.jhipster.lite.module.domain.JHipsterModule;
import tech.jhipster.lite.module.domain.npm.NpmLazyInstaller;
import tech.jhipster.lite.module.domain.properties.JHipsterModuleProperties;

@Service
public class VueApplicationService {

private final VueModulesFactory factory;
private final NpmLazyInstaller npmLazyInstaller;

public VueApplicationService() {
public VueApplicationService(NpmLazyInstaller npmLazyInstaller) {
factory = new VueModulesFactory();
this.npmLazyInstaller = npmLazyInstaller;
}

public JHipsterModule buildVueModule(JHipsterModuleProperties properties) {
return factory.buildVueModule(properties);
return factory.buildVueModule(properties, npmLazyInstaller);
}

public JHipsterModule buildPiniaModule(JHipsterModuleProperties properties) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import tech.jhipster.lite.module.domain.JHipsterModule;
import tech.jhipster.lite.module.domain.file.JHipsterDestination;
import tech.jhipster.lite.module.domain.file.JHipsterSource;
import tech.jhipster.lite.module.domain.npm.NpmLazyInstaller;
import tech.jhipster.lite.module.domain.properties.JHipsterModuleProperties;
import tech.jhipster.lite.module.domain.replacement.MandatoryReplacer;
import tech.jhipster.lite.shared.error.domain.Assert;
Expand Down Expand Up @@ -51,7 +52,7 @@ public class VueModulesFactory {
app.use(pinia);
""";

public JHipsterModule buildVueModule(JHipsterModuleProperties properties) {
public JHipsterModule buildVueModule(JHipsterModuleProperties properties, NpmLazyInstaller npmLazyInstaller) {
//@formatter:off
return moduleBuilder(properties)
.preCommitActions(stagedFilesFilter("{src/**/,}*.{ts,vue}"), preCommitCommands("eslint --fix", "prettier --write"))
Expand Down Expand Up @@ -80,6 +81,9 @@ public JHipsterModule buildVueModule(JHipsterModuleProperties properties) {
.addScript(scriptKey("start"), scriptCommand("vite"))
.addScript(scriptKey("watch:tsc"), scriptCommand("npm run build:tsc -- --watch"))
.and()
.postActions()
.add(context -> npmLazyInstaller.runInstallIn(context.projectFolder()))
.and()
.files()
.add(SOURCE.file("tsconfig.build.json"), to("tsconfig.build.json"))
.batch(SOURCE, to("."))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
import org.springframework.stereotype.Service;
import tech.jhipster.lite.generator.prettier.domain.PrettierModuleFactory;
import tech.jhipster.lite.module.domain.JHipsterModule;
import tech.jhipster.lite.module.domain.npm.NpmLazyInstaller;
import tech.jhipster.lite.module.domain.properties.JHipsterModuleProperties;

@Service
public class PrettierApplicationService {

private final PrettierModuleFactory factory;
private final NpmLazyInstaller npmLazyInstaller;

public PrettierApplicationService() {
factory = new PrettierModuleFactory();
public PrettierApplicationService(NpmLazyInstaller npmLazyInstaller) {
this.npmLazyInstaller = npmLazyInstaller;
this.factory = new PrettierModuleFactory();
}

public JHipsterModule buildModule(JHipsterModuleProperties properties) {
return factory.buildModule(properties);
return factory.buildModule(properties, npmLazyInstaller);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
import tech.jhipster.lite.module.domain.JHipsterModule;
import tech.jhipster.lite.module.domain.file.JHipsterDestination;
import tech.jhipster.lite.module.domain.file.JHipsterSource;
import tech.jhipster.lite.module.domain.npm.NpmLazyInstaller;
import tech.jhipster.lite.module.domain.properties.JHipsterModuleProperties;

public class PrettierModuleFactory {

private static final JHipsterSource SOURCE = from("prettier");
private static final JHipsterDestination DESTINATION = to(".");

public JHipsterModule buildModule(JHipsterModuleProperties properties) {
public JHipsterModule buildModule(JHipsterModuleProperties properties, NpmLazyInstaller npmLazyInstaller) {
//@formatter:off
return moduleBuilder(properties)
.context()
Expand All @@ -43,6 +44,9 @@ public JHipsterModule buildModule(JHipsterModuleProperties properties) {
.addScript(scriptKey("prettier:check"), scriptCommand("prettier --check ."))
.addScript(scriptKey("prettier:format"), scriptCommand("prettier --write ."))
.and()
.postActions()
.add(context -> npmLazyInstaller.runInstallIn(context.projectFolder()))
.and()
.preCommitActions(stagedFilesFilter("*.{md,json,yml,html,css,scss,java,xml,feature}"), preCommitCommands("['prettier --write']"))
.build();
//@formatter:on
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package tech.jhipster.lite.module.domain.npm;

import tech.jhipster.lite.module.domain.properties.JHipsterProjectFolder;

/**
* Run npm install if a previous npm install has already been done.
*/
public interface NpmLazyInstaller {
void runInstallIn(JHipsterProjectFolder folder);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package tech.jhipster.lite.module.infrastructure.secondary.npm;

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.concurrent.TimeUnit;
import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;
import tech.jhipster.lite.module.domain.npm.NpmLazyInstaller;
import tech.jhipster.lite.module.domain.properties.JHipsterProjectFolder;
import tech.jhipster.lite.shared.error.domain.Assert;
import tech.jhipster.lite.shared.npmdetector.infrastructure.secondary.NpmInstallationReader;
import tech.jhipster.lite.shared.npmdetector.infrastructure.secondary.NpmInstallationType;

/**
* Launches npm install if the npm command is detected and if an existing package-lock.json is present.
*/
@Service
class FileSystemNpmLazyInstaller implements NpmLazyInstaller {

private static final Logger log = LoggerFactory.getLogger(FileSystemNpmLazyInstaller.class);
private final NpmInstallationReader npmInstallationReader = new NpmInstallationReader();

public void runInstallIn(JHipsterProjectFolder folder) {
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");

return;
}

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 ?

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 ?

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");

break;
}
}

private void execute(JHipsterProjectFolder path, String... commands) {
try {
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

}
}

private File folderFile(JHipsterProjectFolder path) {
return new File(path.get());
}

private boolean failedExecution(Process process) throws InterruptedException {
return !process.waitFor(1, TimeUnit.MINUTES);
}

private void traceProcess(String command, Process process) throws IOException {
if (log.isTraceEnabled()) {
log.trace("{}: {}", command, read(process.getInputStream()));
}

if (log.isErrorEnabled()) {
String errors = read(process.getErrorStream());

if (StringUtils.isNotBlank(errors)) {
log.error("Error during {}: {}", command, errors);
}
}
}

private String read(InputStream stream) throws IOException {
return new String(stream.readAllBytes(), StandardCharsets.UTF_8).intern();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package tech.jhipster.lite.module.infrastructure.secondary.npm;

import tech.jhipster.lite.shared.error.domain.ErrorKey;

enum NpmErrorKey implements ErrorKey {
INSTALL_ERROR("install-error");

private final String key;

NpmErrorKey(String key) {
this.key = key;
}

@Override
public String get() {
return key;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package tech.jhipster.lite.module.infrastructure.secondary.npm;

import tech.jhipster.lite.shared.error.domain.GeneratorException;
import tech.jhipster.lite.shared.generation.domain.ExcludeFromGeneratedCodeCoverage;

@ExcludeFromGeneratedCodeCoverage
class NpmInstallException extends GeneratorException {

public NpmInstallException(String message) {
super(internalServerError(NpmErrorKey.INSTALL_ERROR).message(message));
}

public NpmInstallException(String message, Throwable cause) {
super(internalServerError(NpmErrorKey.INSTALL_ERROR).message(message).cause(cause));
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.ImportRuntimeHints;
import tech.jhipster.lite.shared.npmdetector.infrastructure.secondary.NpmInstallationReader;

@Configuration
@ImportRuntimeHints(NativeHints.class)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package tech.jhipster.lite.project.infrastructure.secondary;
package tech.jhipster.lite.shared.npmdetector.infrastructure.secondary;

import java.io.IOException;
import java.util.concurrent.TimeUnit;
Expand All @@ -9,7 +9,7 @@

@Service
@ExcludeFromGeneratedCodeCoverage(reason = "Cases can only be tested by using different computers")
class NpmInstallationReader {
public class NpmInstallationReader {

private static final Logger log = LoggerFactory.getLogger(NpmInstallationReader.class);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package tech.jhipster.lite.shared.npmdetector.infrastructure.secondary;

public enum NpmInstallationType {
NONE,
UNIX,
WINDOWS,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@tech.jhipster.lite.SharedKernel
package tech.jhipster.lite.shared.npmdetector;
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,6 @@ error.unknown-slug.title=Unknown slug

error.unmappable-enum.detail=Can't map enum value
error.unmappable-enum.title=Unmappable enum

error.install-error.detail=An error happened while installing npm dependencies
error.install-error.title=Error while installing npm dependencies
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,6 @@ error.unknown-slug.title=Slug inconnu

error.unmappable-enum.detail=Valeur d'enum non mappable
error.unmappable-enum.title=Enum in-mappable

error.install-error.detail=Une erreur s'est produite lors de l'installation des dépendances npm
error.install-error.title=Erreur lors de l'installation des dépendances npm
Original file line number Diff line number Diff line change
@@ -1,27 +1,44 @@
package tech.jhipster.lite.generator.client.vue.core.domain;

import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.*;
import static org.mockito.Mockito.verify;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.ModuleFile;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.assertThatModuleWithFiles;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.eslintConfigFile;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.lintStagedConfigFile;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.nodeDependency;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.nodeScript;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.packageJsonFile;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.tsConfigFile;
import static tech.jhipster.lite.module.infrastructure.secondary.JHipsterModulesAssertions.vitestConfigFile;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import tech.jhipster.lite.TestFileUtils;
import tech.jhipster.lite.UnitTest;
import tech.jhipster.lite.module.domain.JHipsterModule;
import tech.jhipster.lite.module.domain.JHipsterModulesFixture;
import tech.jhipster.lite.module.domain.npm.NpmLazyInstaller;
import tech.jhipster.lite.module.domain.properties.JHipsterModuleProperties;

@UnitTest
@ExtendWith(MockitoExtension.class)
class VueModulesFactoryTest {

private static final VueModulesFactory factory = new VueModulesFactory();

@Mock
private NpmLazyInstaller npmLazyInstaller;

@Test
void shouldCreateVueModule() {
JHipsterModuleProperties properties = JHipsterModulesFixture.propertiesBuilder(TestFileUtils.tmpDirForTest())
.projectBaseName("jhiTest")
.basePackage("tech.jhipster.jhlitest")
.build();

JHipsterModule module = factory.buildVueModule(properties);
JHipsterModule module = factory.buildVueModule(properties, npmLazyInstaller);

//@formatter:off
assertThatModuleWithFiles(module, packageJsonFile(), lintStagedConfigFile(), tsConfigFile(), vitestConfigFile(), eslintConfigFile())
Expand Down Expand Up @@ -80,6 +97,7 @@ void shouldCreateVueModule() {
.hasFiles("src/test/webapp/unit/shared/http/infrastructure/secondary/AxiosStub.ts")
.hasFiles("src/test/webapp/unit/router/infrastructure/primary/HomeRouter.spec.ts");
//@formatter:on
verify(npmLazyInstaller).runInstallIn(properties.projectFolder());
}

@Test
Expand Down
Loading
Loading