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.

Sign up for GitHub

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

Jump to bottom

fix: Also take into account the application.properties value #19966

Closed
wants to merge 2 commits into from
Closed

fix: Also take into account the application.properties value #19966

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25f9afb
fix: Also take into account the application.properties value
caalador Sep 17, 2024
de06dd1
fix gradle call
caalador Sep 17, 2024
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
3 changes: 2 additions & 1 deletion ...ns/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FlowModeAbstractMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,8 @@ public boolean isReactEnabled() {
return reactEnable;
}
File frontendDirectory = BuildFrontendUtil.getFrontendDirectory(this);
return FrontendUtils.isReactRouterRequired(frontendDirectory);
return FrontendUtils.isReactRouterRequired(frontendDirectory,
new File(projectBaseDirectory().toFile(), buildFolder()));
}

@Override
Expand Down
29 changes: 28 additions & 1 deletion flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Properties;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -1266,7 +1267,8 @@ public static File getFlowGeneratedWebComponentsFolder(
* path to the frontend folder in a project.
* @return {@code false} if vaadin-router is used, {@code true} otherwise.
*/
public static boolean isReactRouterRequired(File frontendDirectory) {
public static boolean isReactRouterRequired(File frontendDirectory,
File buildDirectory) {
Objects.requireNonNull(frontendDirectory);
boolean result = true;
File indexTs = new File(frontendDirectory, FrontendUtils.INDEX_TS);
Expand All @@ -1282,13 +1284,38 @@ public static boolean isReactRouterRequired(File frontendDirectory) {
e);
}
}
if (result) {
// If we are enabling react we should for spring projects check the
// application.properties file flag also.
File resource = new File(buildDirectory,
"classes/application.properties");
Copy link
Contributor

Choose a reason for hiding this comment

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

application.yml/yaml are not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more of a test at the moment and as flow doesn't have any yaml handling libraries it would be a build by hand parsing.
Also not certain if the linking to spring specific property files is a good idea as it might lead to expectations in overall configuration.

Copy link
Member

Choose a reason for hiding this comment

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

This would also leave out all profile specific properties files application-{profile}.properties like application-prod.properties, application-dev.properties etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that's also true.. I'm wondering if there could be some kind of way to get the available spring environment in the build so that spring would handle all their "property magic"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main problem with possibly trying to use spring tools is that the data is needed during plugin build time also and not just runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One possibility could be to add to starter projects into pom.xml <properties><reactEnable>true</reactEnable></properties> and have in the default application.properties vaadin.react.enable=@reactEnable@.
This would sync the pom.xml and application.properties values for the flag.

The problem here is ofc that it could be removed by mistake and wouldn't help old projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this require additional configuration? Like filtering of resources to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it works out of the box for spring-boot: https://docs.spring.io/spring-boot/docs/2.0.0.M7/reference/html/howto-properties-and-configuration.html#howto-automatic-expansion-maven

If you use the spring-boot-starter-parent, you can then refer to your Maven ‘project properties’ with @..@ placeholders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the starter parent you would need resource filtering and plugin delimiter configuration.

Copy link
Member

Choose a reason for hiding this comment

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

vaadin.react.enable=@reactEnable@ looks good idea, but polluting starters with a property that most don't need is not so nice.
If the point is to avoid issues with vaadin.react.enable being added only to application.properties then better documentation could be enough. Also this @reactEnable@ should be mentioned somewhere.

if (resource.exists()) {
try (InputStream resourceStream = resource.toURL()
.openStream()) {
Properties prop = new Properties();
prop.load(resourceStream);
if (prop.containsKey("vaadin.react.enable")) {
result = Boolean.getBoolean(
prop.get("vaadin.react.enable").toString());
}
} catch (IOException e) {
getLogger().error(
"Couldn't read application.properties for react flag, react-router will be used",
e);
}
}
}
if (getLogger().isDebugEnabled()) {
getLogger().debug("Auto-detected client-side router to use: {}",
result ? "react-router" : "vaadin-router");
}
return result;
}

protected static ClassLoader getClassLoader() {
return FrontendUtils.class.getClassLoader();
}

/**
* Auto-detects if hilla views are used in the project based on what is in
* routes.ts or routes.tsx file.
Expand Down
5 changes: 3 additions & 2 deletions flow-server/src/main/java/com/vaadin/flow/server/frontend/Options.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,8 +930,9 @@ public boolean isReactEnabled() {

public Options withReact(boolean reactEnable) {
this.reactEnable = reactEnable;
if (reactEnable && !FrontendUtils
.isReactRouterRequired(getFrontendDirectory())) {
if (reactEnable
&& !FrontendUtils.isReactRouterRequired(getFrontendDirectory(),
getBuildDirectory())) {
LoggerFactory.getLogger(Options.class).debug(
"Setting reactEnable to false as Vaadin Router is used!");
this.reactEnable = false;
Expand Down
3 changes: 2 additions & 1 deletion ...er/src/main/java/com/vaadin/flow/server/frontend/scanner/FrontendDependenciesScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ public FrontendDependenciesScanner createScanner(

public FrontendDependenciesScanner createScanner(Options options) {
boolean reactEnabled = options.isReactEnabled() && FrontendUtils
.isReactRouterRequired(options.getFrontendDirectory());
.isReactRouterRequired(options.getFrontendDirectory(),
options.getBuildDirectory());
return createScanner(!options.isUseByteCodeScanner(),
options.getClassFinder(),
options.isGenerateEmbeddableWebComponents(),
Expand Down
45 changes: 42 additions & 3 deletions flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -505,7 +506,7 @@ public void isReactRouterRequired_importsVaadinRouter_false()
router.setRoutes(routes);
""");
Assert.assertFalse("vaadin-router expected when it imported",
FrontendUtils.isReactRouterRequired(frontend));
FrontendUtils.isReactRouterRequired(frontend, frontend));
}

@Test
Expand All @@ -521,14 +522,52 @@ public void isReactRouterRequired_doesntImportVaadinRouter_true()
""");
Assert.assertTrue(
"react-router expected when no vaadin-router imported",
FrontendUtils.isReactRouterRequired(frontend));
FrontendUtils.isReactRouterRequired(frontend, frontend));
}

@Test
public void isReactRouterRequired_noIndexTsFile_true() throws IOException {
File frontend = tmpDir.newFolder(FrontendUtils.DEFAULT_FRONTEND_DIR);
Assert.assertTrue("react-router expected when index.ts isn't there",
FrontendUtils.isReactRouterRequired(frontend));
FrontendUtils.isReactRouterRequired(frontend, frontend));
}

@Test
public void isReactRouterRequired_applicationPropertiesHasFalse_false()
throws IOException {
File frontend = prepareFrontendForRoutesFile(FrontendUtils.INDEX_TS,
"""
import { createElement } from "react";
import { createRoot } from "react-dom/client";
import App from "./App.js";

createRoot(document.getElementById("outlet")!).render(createElement(App));
""");
File applicationProperties = new File(frontend,
"classes/application.properties");
FileUtils.write(applicationProperties, "vaadin.react.enable=false");
Assert.assertFalse(
"react-router expected when no vaadin-router imported",
FrontendUtils.isReactRouterRequired(frontend, frontend));
}

@Test
public void isReactRouterRequired_applicationPropertiesTrueDoesNotOverride_false()
throws IOException {
File frontend = prepareFrontendForRoutesFile(FrontendUtils.INDEX_TS,
"""
import { Router } from '@vaadin/router';
import { routes } from './routes';

export const router = new Router(document.querySelector('#outlet'));
router.setRoutes(routes);
""");
File applicationProperties = new File(frontend,
"classes/application.properties");
FileUtils.write(applicationProperties, "vaadin.react.enable=true");
Assert.assertFalse(
"react-router expected when no vaadin-router imported",
FrontendUtils.isReactRouterRequired(frontend, frontend));
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdaterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,9 +568,9 @@ public void getDefaultDependencies_reactIsUsed_addsHillaReactComponents() {
.mockStatic(FrontendUtils.class)) {
mock.when(() -> FrontendUtils.isHillaUsed(Mockito.any(File.class),
Mockito.any(ClassFinder.class))).thenReturn(true);
mock.when(() -> FrontendUtils
.isReactRouterRequired(Mockito.any(File.class)))
.thenReturn(true);
mock.when(() -> FrontendUtils.isReactRouterRequired(
options.getFrontendDirectory(),
options.getBuildDirectory())).thenReturn(true);
options.withReact(true);
Map<String, String> defaultDeps = nodeUpdater
.getDefaultDependencies();
Expand Down
5 changes: 3 additions & 2 deletions vaadin-dev-server/src/main/java/com/vaadin/base/devserver/startup/DevModeInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,9 @@ public static DevModeHandler initDevModeHandler(Set<Class<?>> classes,
JsonObject tokenFileData = Json.createObject();
Mode mode = config.getMode();
boolean reactEnable = config.getBooleanProperty(REACT_ENABLE,
FrontendUtils
.isReactRouterRequired(options.getFrontendDirectory()));
FrontendUtils.isReactRouterRequired(
options.getFrontendDirectory(),
options.getBuildDirectory()));
options.enablePackagesUpdate(true)
.useByteCodeScanner(useByteCodeScanner)
.withFrontendGeneratedFolder(frontendGeneratedFolder)
Expand Down
Loading