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

Add UI and API docs custom context path support #12923

Closed

Conversation

PierreBesson
Copy link
Contributor

Depend on #jhipster/jhipster-bom#42
Fixes #12649

These changes follow @clocken's great suggestions and analysis for supporting a custom context-path for JHipster apps including UI and openapi docs support.

How to test

export SERVER_SERVLET_CONTEXT_PATH=/myapp
mvn || gradle

TODO:

  • update docs (relating to custom path and war support)

@CLAassistant
Copy link

CLAassistant commented Nov 1, 2020

CLA assistant check
All committers have signed the CLA.

@DanielFran
Copy link
Member

@PierreBesson It seems you have conflicts and you need to sign CLA also

@PierreBesson
Copy link
Contributor Author

@clocken Could you sign the CLA please as I have included your commit. Thanks!

@PierreBesson
Copy link
Contributor Author

As for the conflict, I am unable to see it locally 🤔.

@pascalgrimaud
Copy link
Member

@PierreBesson : check with your main branch, instead of master branch ;)

@PierreBesson
Copy link
Contributor Author

PierreBesson commented Nov 1, 2020

I had totally forgot that it's not the same.

@clocken
Copy link

clocken commented Nov 9, 2020

@PierreBesson Thanks for the implementation but it looks like we had a bit of a "race condition" here.
In the meantime the client app has switched to Angular CLI and the webpack.common.js.ejs has been removed - see 805e5cc.
I accounted for that in a copy of your branch and also removed the quotes around the default value for the management-include-pattern. I also found two more places for relative paths in the React and Vue clients.
Feel free to cherry-pick again from my branch:
clocken/ui-context-path-support

PS: PR for doc update is here: jhipster.github.io#1030

@ruddell
Copy link
Member

ruddell commented Nov 13, 2020

Before merging, please make sure a path with multiple slashes (such as /admin/metrics) is tested both with and without a context-path. This includes refreshing or directly navigating to the path. This will fail if the context-path is relative (like in @clocken's changes).

Without the changes from @clocken, this PR causes the initial page load to fail in prod (attempts to find /main.js instead of /myapp/main.js).

@PierreBesson
Copy link
Contributor Author

I think the PR is finalized now. Let's validate those cases (including the metrics page as @ruddell pointed out) :

Framework UI without context-path UI with context-path
Angular
React
Vue

Note: the control center should also be checked for context-path support.

@clocken
Copy link

clocken commented Dec 18, 2020

Ok, I'm done testing now. The base-href has to be reverted back to an absolute path and set to a context path manually to be able to page refresh or directly navigate to paths with multiple slashes, changes ready for cherry-picking in d072af0 @PierreBesson. I also updated the docs accordingly.
With that small revert all cases tested positive:

Framework UI without context-path UI with context-path
Angular ✔️ ✔️
React ✔️ ✔️
Vue ✔️ ✔️

@PierreBesson
Copy link
Contributor Author

I have done extensive testing and rebased to main. I confirm @clocken tests and everything should be fine now. However CI does seem to have issues due to loading jhipster dependencies 7.0.0-beta and not the 7.0.0-SNAPSHOT from the context-path branch. Maybe this is related to the current in-progress release.

@pascalgrimaud if this is not included in the first v7 beta, it can be included in the next beta version.

@pascalgrimaud
Copy link
Member

yes, I think it's better to include it in beta.1
The beta.0 is for tomorrow, I prefer to secure the release

@mraible
Copy link
Contributor

mraible commented Jan 13, 2021

Any updates @PierreBesson?

clocken and others added 9 commits March 6, 2021 14:22
 * inject <base href="./"> instead of <base href="/"> into index.html via Webpack
 * use '../swagger-resources' for axios-request in SwaggerUI
 * use '..' + resource.location for Resource-URLs displayed in SwaggerUI
…es to take into account the application context-path

fixes jhipster#12649
Directly set it in the template and remove webpack.common.js.ejs as client app now uses AngularCLI.
Be consistent with the default api doc pattern :)
Emit relative paths for hashed resources from webpack asset modules
Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
@PierreBesson
Copy link
Contributor Author

index.html moved to https://github.com/jhipster/generator-jhipster/blob/main/generators/client/templates/common/src/main/webapp/index.html.ejs

swagger-ui/index.html moved to https://github.com/jhipster/generator-jhipster/blob/main/generators/client/templates/common/src/main/webapp/swagger-ui/index.html.ejs.

They should be dropped and any changes should be implemented there.

Yes I have also noticed this. Thanks !

@PierreBesson
Copy link
Contributor Author

I am currently not able to make this works on my machine. This is not such a big change but it might be impacted by some recent changes in spring boot 2.4, I'm not sure.
I repeat myself but this would be really nice to have for microservices that use a context-path.

@pascalgrimaud
Copy link
Member

There are several major changes with Spring Boot 2.4:

We had to adapt our code, to change the result from API, it was not easy but I'm confident it's done today.
For adding support for context path, we need to look at these 2 PRs and do similar change. It would be with the same complexity.

Important thing, about context path. You can easily add this property, but there will be several things which are broken:

  • npm start, will be broken
  • swagger
  • logout with OAuth2
  • deployment
  • etc

I forgot probably some items, as I already did this for my customer. And as I said, I'm not sure we can automate this, because the property comes from Spring Boot property and I needed to put this context path in some .vue / .ts file. These files can't guess this property.
So adding this support is difficult IMO :-)

@DanielFran
Copy link
Member

@PierreBesson @pascalgrimaud Following #14250, this PR should be closed?

@pascalgrimaud pascalgrimaud added this to the v7.0.0 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploying JHipster with a context path breaks SwaggerUI
8 participants