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

Beginnings of the master single page application. #2898

Closed
wants to merge 1 commit into from

Conversation

@joeldenning
Copy link

commented May 16, 2019

Description of what I changed

See https://github.com/openmrs/openmrs-rfc-frontend

There is a substantial risk to merging in this pull request, because it changes to default behavior for all routes in openmrs. I was unable to test that frontend routes for legacyui, adminui, refapp are still working, which is important. Reason is that I couldn't get those modules to boot up correctly locally.

Checklist

  • My pull request only contains ONE single commit

  • My IDE is configured to follow the code style of this project.

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

  • My pull request is based on the latest changes of the master branch.

@@ -306,6 +306,15 @@
<servlet-name>jsp</servlet-name>
<url-pattern>*.jsp</url-pattern>
</servlet-mapping>

<servlet>
<servlet-name>default</servlet-name>

This comment has been minimized.

Copy link
@joeldenning

joeldenning May 16, 2019

Author

See https://tomcat.apache.org/tomcat-7.0-doc/default-servlet.html.

This is why this PR has such a big impact. Any route not covered by any of the other servlets will end up hitting this route.

@coveralls

This comment has been minimized.

Copy link

commented May 16, 2019

Coverage Status

Coverage decreased (-0.1%) to 59.714% when pulling ae492bf on joeldenning:master-spa into 61178e8 on openmrs:master.

@joeldenning joeldenning force-pushed the joeldenning:master-spa branch 3 times, most recently from f797359 to 852c5c3 May 17, 2019

@joeldenning joeldenning force-pushed the joeldenning:master-spa branch from 852c5c3 to ae492bf May 17, 2019

@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) {
try {
RequestDispatcher dispatcher = request.getRequestDispatcher("/master-single-page-application.jsp");

This comment has been minimized.

Copy link
@mseaton

mseaton May 20, 2019

Member

@joeldenning If we are worried about the impact of this in the short term, we could make this configurable. eg. If openmrs-runtime.properties file contains "enable-single-page-application=true", then execute this dispatch, otherwise fall back to legacy behavior

@bmamlin

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Does the SPA servlet have to cast such a broad net – i.e., are there specific requests we are expecting that would not be made to /master-single-page-application.jsp* that need to be handled by the SPA servlet?

I was assuming a SPA, by design, would be using URLs in a pattern like {PATH_TO_SINGLE_SPA_MASTER}#/the/path/for/spa/routers. If all calls to the SPA are expected to fall under a predictable path (the master page), then it seems far less risky to introduce an OpenMRS SPA module that could own {OPENMRS}/module/spa and everything below it. In the future, we could support {OPENMRS}/spa (like we've done with {OPENMRS}/ws for web services).

@joeldenning, if Single SPA can work as long as it receives all calls to a specific path or below, then we could create a simple OpenMRS SPA module for you that could serve as the handler for all SPA traffic.

@dkayiwa

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

I have made a commit to the SPA module, which has an overridable spa.baseUrl setting that defaults to {OPENMRS}/spa
openmrs/openmrs-module-spa@8df90c7

@joeldenning if it works for you, then we can cancel this pull request.

@joeldenning

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

Yep 👍 thanks for your help with openmrs-module-spa

@dkayiwa dkayiwa referenced this pull request Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.