-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
replace default server port when property has custom value #8825
replace default server port when property has custom value #8825
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I made a few suggestions to simplify things
@@ -132,7 +132,7 @@ public JHipsterModule buildTailwindcssModule(JHipsterModuleProperties properties | |||
.packageJson() | |||
.addDevDependency(packageName("tailwindcss"), COMMON) | |||
.addScript(scriptKey("watch:html"), scriptCommand("onchange 'src/main/resources/templates/**/*.html' -- npm-run-all --serial build:css build:html")) | |||
.addScript(scriptKey("watch:serve"), scriptCommand("browser-sync start --no-inject-changes --proxy localhost:8080 --files 'target/classes/templates' 'target/classes/static'")) | |||
.addScript(scriptKey("watch:serve"), scriptCommand("browser-sync start --no-inject-changes --proxy localhost:%s --files 'target/classes/templates' 'target/classes/static'".formatted(getServerPort(properties)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason to redefine the default 8080 port in every module, it's already defined in JHipsterServerPort so you can just use:
.addScript(scriptKey("watch:serve"), scriptCommand("browser-sync start --no-inject-changes --proxy localhost:%s --files 'target/classes/templates' 'target/classes/static'".formatted(getServerPort(properties)))) | |
.addScript(scriptKey("watch:serve"), scriptCommand("browser-sync start --no-inject-changes --proxy localhost:%s --files 'target/classes/templates' 'target/classes/static'".formatted(properties.serverPort().get()))) |
and remove the getServerPort()
method below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I was not sure if the serverPort property is generated as well when no spring-boot-mvc-empty module is added. I have removed unnecessary method
@@ -100,11 +101,18 @@ public JHipsterModule buildModule(JHipsterModuleProperties properties) { | |||
.in(path("package.json")) | |||
.add(lineBeforeText(CACHE_NEEDLE), jestSonar(properties.indentation())) | |||
.and() | |||
.in(path("proxy.conf.json")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than doing a replacement, you should turn proxy.conf.json
to a template that will use the serverPort
properties:
- rename
proxy.conf.json
in src/main/resources/generator/client/angular/core/ toproxy.conf.json.mustache
and replace8080
by{{serverPort}}
- change line
.add(SOURCE.file("proxy.conf.json"), to("proxy.conf.json"))
to.addTemplate(SOURCE.file("proxy.conf.json"), to("proxy.conf.json"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I have did the same to vte.config.ts in React Module. However, as I was not able to add the addTemple without the batch, I had to modify it to:
...
.batch(SOURCE, to("."))
.addTemplate("proxy.conf.json")
.and()
...
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8825 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 2819 2819
===========================================
Files 719 719
Lines 12409 12411 +2
Branches 250 250
===========================================
+ Hits 12409 12411 +2 ☔ View full report in Codecov by Sentry. |
Congrats @alerGeek for your first contribution :-) |
PR fix frontend properties:
PR fix README.md files/properties for ouath docker image.
Fix #8824