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

feat: deduce base url from hosted location [LIBS-580] #832

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KaiVandivier
Copy link
Contributor

https://dhis2.atlassian.net/browse/LIBS-580

Computes a base URL from the location the app is hosted. This should be relatively stable, and there are only currently two possibilities (there may be more with the global shell work)

Tested out existing cases to make sure they still work by deploying a build of the simple example app (custom app) and by injecting this build of the Adapter into a core app and deploying that (with a console log added), but have not yet done a test of bundling a previously unbundled app

Screenshot 2024-02-23 at 8 22 12 PM Screenshot 2024-02-23 at 8 40 42 PM

@KaiVandivier KaiVandivier marked this pull request as ready for review February 23, 2024 20:11
Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

Looks good, great work Kai!

@janhenrikoverland @Philip-Larsen-Donnelly this will allow a single build of an app (i.e. Line Listing) to be served as EITHER bundled or installed app

I haven't tested this myself yet. We will need to also update the backend to not enforce coreApp declarations in the manifest when apps are installed.

There's a slight risk of misdetecting the baseUrl in some rare situations if DHIS2 is served at a path including /api/apps for example (or with dhis-web in installed app names) but I think this is probably ok...

Comment on lines 5 to 12
const testPaths = {
'/dev/api/apps/simple-app/index.html': '/dev/',
'/analytics_dev/dhis-web-maps/plugin.html': '/analytics_dev/',
'/dhis-web-line-listing/index.html': '/',
'/dhis-web-user-settings/': '/',
'/hmis/staging/v41/api/apps/WHO-Data-Quality-App/app.html':
'/hmis/staging/v41/',
}
Copy link
Member

Choose a reason for hiding this comment

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

Another ambiguous path we might need to consider...

Suggested change
const testPaths = {
'/dev/api/apps/simple-app/index.html': '/dev/',
'/analytics_dev/dhis-web-maps/plugin.html': '/analytics_dev/',
'/dhis-web-line-listing/index.html': '/',
'/dhis-web-user-settings/': '/',
'/hmis/staging/v41/api/apps/WHO-Data-Quality-App/app.html':
'/hmis/staging/v41/',
}
const testPaths = {
'/dev/api/apps/simple-app/index.html': '/dev/',
'/analytics_dev/dhis-web-maps/plugin.html': '/analytics_dev/',
'/dhis-web-line-listing/index.html': '/',
'/dhis-web-user-settings/': '/',
'/hmis/staging/v41/api/apps/WHO-Data-Quality-App/app.html':
'/hmis/staging/v41/',
'/api/apps/dhis-web-maps': '/api/apps',
}

An installed app could have dhis-web-maps as a name. We should probably block installation of apps with that prefix in core...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm yeah that's an interesting case -- at /api/apps/dhis-web-maps/, we could either be at a weird custom app with the name 'dhis-web-maps' at a base URL of /, OR we could be at the core Maps app in an instance weirdly hosted with the base URL /api/apps/... I'm not sure if it's possible to figure out which case is true here hah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could ping the server to see which base URL works 🤷 or we could decide that this will always assume that's a custom app

Copy link
Member

Choose a reason for hiding this comment

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

I think we could add enforcement in app hub and core app install logic to disallow custom apps with the dhis-web prefix, hopefully that would be sufficient to disambiguate these cases? And I don't think we need to cater to apps really wanting that specific URL slug...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, added that test case 🙂 👍

@amcgee amcgee self-requested a review February 25, 2024 22:43
@amcgee
Copy link
Member

amcgee commented Feb 25, 2024

I'll test this before approving.

@KaiVandivier
Copy link
Contributor Author

Hm, I noticed that these new URLs end with a /, which seems to deviate from our current practice in some places -- here are some observations:

  • One place that was affected is the connection status ping query, which would be a small breaking change
  • A trailing / results in errors in development (see screenshot below)
  • Custom apps are sensitive to the final /: api/apps/line-listing returns a 404, but api/apps/line-listing/ returns the app (core apps seem to be flexible

Is one of these more correct than the other? 🤔 (Ending with a / seems more correct to me) Should I change these URLs to end without a / to avoid any breakages? Or, if an ending / is more correct, should we move towards that?

Screenshot
example-base-url

@amcgee
Copy link
Member

amcgee commented Mar 21, 2024

@KaiVandivier they should always end with a /, which is why ../ and ../../../ work (respectively) in the current setup. The bundled apps are flexible because the core responds to requests for /dhis-web-dashboard with a 302 reidrect telling the browser to go to /dhis-web-dashboard/. So we can assume we always have the / I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants