O3-3438: Update SPA module to load the bundled frontend #62
Conversation
| File file = new File(openmrsRootPath, "WEB-INF" + File.separator + spaDirectory); | ||
| SpaDirectoryResolver.spaDirectory.set(Paths.get(file.toURI()).normalize().toAbsolutePath().toString()); |
There was a problem hiding this comment.
Why are we using both File and Paths here? Just use one or the other (Paths.get(openmrsRootPath, 'WEB-INF', spaDirectory).toAbsolutePath().toString())
|
|
||
| public SpaDirectoryResolver(ServletContext servletContext) { | ||
| this.servletContext = servletContext; | ||
| if (isBundledFrontend()){ |
There was a problem hiding this comment.
This kind of forces things to use the bundled frontend if one exists. I think a better solution here would be:
- At module start, check to see if the local directory exists
- If it does, there's no work to do
- If it doesn't and there is a bundled frontend, extract it to the local directory
That way all the work is done in the Activator.
|
@ibacher I updated this PR. |
| @@ -75,6 +85,12 @@ private static void resolveDirectory(String spaDirectory) { | |||
| return; | |||
| } | |||
|
|
|||
| if(spaDirectory.equals(BUNDLED_FRONTEND_DIRECTORY)){ | |||
There was a problem hiding this comment.
Why go through all of this when, in the Activator we can just set the GP to the resolved path to the bundled frontend directory?
| @Override | ||
| public void started() { | ||
| log.info("SPA module started"); | ||
| spaDirectoryResolver = new SpaDirectoryResolver(); | ||
| File indexHtml = new File(OpenmrsUtil.getApplicationDataDirectory() + File.separator + SpaConstants.DEFAULT_FRONTEND_DIRECTORY, "index.html"); |
There was a problem hiding this comment.
This code should be moved to the contextRefreshed() hook instead of the started() hook, like in the OWA Activator.
There was a problem hiding this comment.
Additionally, it seems like we could just do:
SpaDirectoryResolver directoryResolver = new SpaDirectoryResolver();
if (!Paths.get(directoryResolver.getSpaDirectory(), "index.html").toFile().exists()) {
Path bundledFrontend = Paths.get(servletContext.getRealPath("/"), "WEB-INF", BUNDLED_FRONTEND_DIRECTORY);
if (bundledFrontend.resolve("index.html").toFile().exist()) {
// activator methods are always run in the daemon context, so it has all privileges
Context.getAdministrationService().setGlobalProperty(GP_LOCAL_DIRECTORY, bundledFrontend.toAbsolutePath().toString());
}
}This would then only require the addition of the SpaActivator as a bean and all the changes to the SpaDirectoryResolver can be removed.
wikumChamith
force-pushed
the
O3-3438
branch
2 times, most recently
from
50c573e to
1db7ddb
Compare
|
@ibacher I updated the PR. |
There was a problem hiding this comment.
Thanks @wikumChamith! Moving in the right direction here, but still looks like we need some changes. Have you tried this code out?
| @Override | ||
| public void started() { | ||
| log.info("SPA module started"); | ||
| spaDirectoryResolver = new SpaDirectoryResolver(); |
There was a problem hiding this comment.
We should still set this value. Otherwise, the call to add the GP listener won't work.
| if (bundledFrontend.resolve("index.html").toFile().exists()) { | ||
| Context.getAdministrationService().setGlobalProperty(GP_LOCAL_DIRECTORY, bundledFrontend.toAbsolutePath().toString()); | ||
| } | ||
| } | ||
| Context.getAdministrationService().addGlobalPropertyListener(spaDirectoryResolver); |
There was a problem hiding this comment.
This code should probably have a guard if it's not in started():
if (spaDirectoryResolver != null) {
Context.getAdministrationService().addGlobalPropertyListener(spaDirectoryResolver);
}
This is because contextRefreshed() is called before started() IIRC.
|
@ibacher, I made a mistake while testing the PR before and didn't notice an error. I updated the PR to fix it. |
This enables the SPA module to serve the front end from a bundled .war file.
Ticket: https://openmrs.atlassian.net/browse/O3-3438