(fix) O3-3370: Properly handle relative imports in development #1258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ibacher
commented
Jan 7, 2025
| const importmap = JSON.parse(importMapDecl.value); | ||
| const spaPathRegEx = new RegExp('^' + spaPath.replace(/[|\\{}()[\]^$+*?.]/g, '\\$&').replace(/-/g, '\\x2d')); |
There was a problem hiding this comment.
This is a hacky implementation of escaping regex metachars since RegExp.escape() is only available in Node 22.
|
Size Change: -742 kB (-10.71%) 👏 Total Size: 6.18 MB
ℹ️ View Unchanged
|
NethmiRodrigo
approved these changes
Jan 7, 2025
There was a problem hiding this comment.
Thank you @ibacher! LGTM! I have a small question about one of the arguments that was removed
|
Not sure, but I think this has broken local dev environments. The following is from running the attachments frontend module locally after bumping the framework and core tooling in Patient Chart:
|
3 tasks
|
Indeed... It looks like somewhere things are getting transformed into http://dev3.openmrs.org//openmrs/spa/openmrs-esm-devtools-app-6.0.3-pre.2625/openmrs-esm-devtools-app.js. |
4 tasks
Twiineenock
pushed a commit
to Twiineenock/openmrs-esm-core
that referenced
this pull request
Jan 14, 2025
…rs#1258) * (fix) O3-3370: Properly handle relative imports in development * Fix doc strings
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
Follow-on for #1032. #1032 tried to add support for services like GitPod by converting URL references for JS files from fully-qualified names to relative names. However, it did this via string manipulation of the URL, which actually breaks things in the case when running against a local server (since the prefix replace of
http://localhostwith""leads to broken URLs like:8081/openmrs-esm-login-app.js, which fail to load).This PR reworks things so that we're actually doing something similar to a browser's origin check (i.e., confirming that we're using the same host, port, and protocol). If the URL for a JS file is on the same origin as the backend, we convert it to a relative URL and if it is not, we leave it as an absolute URL.
Basically what this means is that when running
yarn start, most of the import map will be converted into local links like:But the app that's under development will remain as an absolute URL like this:
This should ensure that things work both in the case when connecting to a remote server via a reverse proxy and that it doesn't break things for local development.
Screenshots
Related Issue
Other