-
Notifications
You must be signed in to change notification settings - Fork 15
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(cli): instance proxy server #635
Conversation
34cb28b
to
a5af15b
Compare
Sorry, @mediremi. I was going over and deleting some temporary comments I put in during my review and accidentally deleted the comment I was responding to (yours). That wasn't intentional. :| |
adapter/src/components/LoginModal.js
Outdated
|
||
export const LoginModal = () => { | ||
const [server, setServer] = useState( | ||
staticUrl || window.localStorage.DHIS2_BASE_URL || '' | ||
staticUrl || defaultUrl || window.localStorage.DHIS2_BASE_URL || '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I accidentally removed a comment by @mediremi here about that the DHIS2_DEFAULT_BASE_URL
can be used for Netlify PR previews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interesting to support a dropdown list of possible backend targets (through the same proxy) here - particularly useful for netlify deploys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could rename it to DHIS2_DEFAULT_BASE_URLS
and make it a comma-delimited string of values? If multiple values are passed we render a dropdown otherwise we render an input field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the LoginModal
changes required for the proxy to work or can we defer adding REACT_APP_DHIS2_DEFAULT_BASE_URL
and defaultUrl
until later ?
The Netlify PR deploys do not use the start
command, they are static builds anyway, so the proxy won't help there right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this: https://github.com/dhis2/app-platform/pull/635/files#diff-bcd4b64d0aa7ede2599b25756aa2cb9c9f0e8000857f033edb963b85eb0f54e9R47
process.env.DHIS2_DEFAULT_BASE_URL = proxyBaseUrl
Is that required for the proxy, or can it be deferred until we work on the Netlify deploy feature ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not required, it's just there to improve the dev UX by prefilling the login modal's server field to the proxy's URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions, this is really awesome @mediremi 🎉
pathname: parsedLocation.pathname.replace( | ||
parsedTarget.pathname, | ||
'' | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this have issues if i.e. the target pathname is /dev
and there's a /dev
somewhere OTHER than at the start of the string? I.e. we should probably replace /^${pathname}/
instead, so that only the first match is replaced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a string is passed to String.prototype.replace
, only the first match is replaced (e.g. 'aaa'.replace('a', 'b') === 'baa'
). For it to replace multiple matches, a regex with the g
flag must be passed ('aaa'.replace(/a/g, 'b') === 'bbb'
).
you specify doesn't have any domain-restricting proxy settings (such as the `SameSite=Lax`). | ||
Make sure that the the URL of your application (`localhost:3000` in this case) | ||
is included in the DHIS2 server's CORS whitelist (System Settings -> Access). If | ||
your server or browser has domain-restricting settings (such as the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually the case in modern Chrome even if the cookies haven't been explicitly set to restrict domains, since they are now SameSite by default (which is also the DHIS2 default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that the DHIS2_DEFAULT_BASE_URL
can be omitted from this PR and introduced as a separate feature once we decide how to approach Netlify PR deploys with the proxy.
If that's possible, I'd like to omit it and then we can go ahead and merge this.
# [8.0.0-beta.5](v8.0.0-beta.4...v8.0.0-beta.5) (2021-09-09) ### Features * **cli:** instance proxy server ([#635](#635)) ([9df387e](9df387e))
🎉 This PR is included in version 8.0.0-beta.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
# [8.0.0](v7.6.6...v8.0.0) (2021-09-20) ### Bug Fixes * **alerts-service:** add tests and align implementation ([dabe477](dabe477)) * **cli:** set test environment to node ([#625](#625)) ([36d311b](36d311b)) * **dependencies:** update app-runtime to v3 ([8777699](8777699)) * set jsdom as default test environment ([#624](#624)) ([2f1ba42](2f1ba42)) ### chore * **deps:** upgrade to 7.0.0 of @dhis2/ui ([b624c9e](b624c9e)) * **deps:** upgrade to styled-jsx 4.x ([8cf9e17](8cf9e17)) ### Features * **app-adapter:** align Alerts component with alerts-service and AlertBar ([bd4564c](bd4564c)) * **cli:** instance proxy server ([#635](#635)) ([9df387e](9df387e)) * bump jest to v27 ([f5015b2](f5015b2)) ### BREAKING CHANGES * **deps:** @dhis2/ui 7.x has dropped support for the deprecated entrypoints @dhis2/ui-core and @dhis2/ui-widgets. Please use @dhis2/ui to import components you need in your app. Everything from core and widgets is available. * **deps:** Upgrade to styled-jsx 4 requires that the application uses a compatible version of @dhis2/ui. * **dependencies:** This updates the app-platform to version 3 of the app-runtime. That means that this version of the app-platform will only work with apps that use version 3 of the app-runtime. * Upgrade Jest to 27.x. Please see for a list of changes: https://jestjs.io/blog/2021/05/25/jest-27
🎉 This PR is included in version 8.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Introduces a proxy server that can be used to test remote instances without relying on cross-site cookies.
Usage: