config: Fetch config from server in frontend #609
Conversation
tahini
force-pushed
the
refactorPreferences
branch
2 times, most recently
from
c5ef595
to
522e620
Compare
tahini
force-pushed
the
refactorPreferences
branch
2 times, most recently
from
a17c33d
to
6b2b8fd
Compare
Instead of having it built into the application at webpack time, it is fetched at load time from the server. This adds an express route named `/config` that returns a Status with the project configuration. The client's entry page now needs to asynchronously fetch it from server using this route and the page is only rendered when the response is back. If there is an error while getting the configuration, the MaintenancePage is displayed. Because the initial frontend configuration is now asynchronous at the application start, the i18n.config is converted to a function, which initializes the i18n object that should be called once the configuration is fetched.
tahini
force-pushed
the
refactorPreferences
branch
from
6b2b8fd
to
49733d3
Compare
| @@ -7,3 +7,4 @@ | |||
| export { default as LoadingPage } from './LoadingPage'; | |||
| export * from './NotFoundPage'; | |||
| export { default as LoginPage } from './LoginPage'; | |||
| export { default as MaintenancePage } from './MaintenancePage'; | |||
There was a problem hiding this comment.
Is this change relevant?
There was a problem hiding this comment.
Yes, it allows to import the MaintenancePage along with the rest instead of a separate import.
There was a problem hiding this comment.
ok, missread as import instead of export
| hasConfig = configOk; | ||
| renderApp(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Should we have a catch or a else to catch (and log) failure here?
There was a problem hiding this comment.
Thanks for approving the direction, but I think this can still be improved. Especially that the application "entry" will have to be updated in Evolution also. I'll dismiss your review for now so it doesn't accidentally gets merged and I'll try to make it better soon.
| @@ -7,3 +7,4 @@ | |||
| export { default as LoadingPage } from './LoadingPage'; | |||
| export * from './NotFoundPage'; | |||
| export { default as LoginPage } from './LoginPage'; | |||
| export { default as MaintenancePage } from './MaintenancePage'; | |||
There was a problem hiding this comment.
Yes, it allows to import the MaintenancePage along with the rest instead of a separate import.
| const renderApp = () => { | ||
| if (!hasRendered) { | ||
| ReactDOM.render(jsx, document.getElementById('app')); | ||
| if (!hasRendered && hasConfig !== undefined && hasFetchedAuth !== undefined) { |
There was a problem hiding this comment.
In retrospect, 2 things I don't like here: the multiple flags and the fact that we need to call a i18n initialization function. It makes the currentLanguage potentially undefined and multiplies the possibilities. I think we should fetch the configuration, then import the rest which we are certain will run with a valid configuration. If we can't get the config, we put a [eventually] pretty and generic failure page.
tahini
dismissed
greenscientist’s stale review
Not ready for merge yet, still can be improved
|
I'll work on #54 first |
Instead of having it built into the application at webpack time, it is fetched at load time from the server.
This adds an express route named
/configthat returns a Status with the project configuration.The client's entry page now needs to asynchronously fetch it from server using this route and the page is only rendered when the response is back. If there is an error while getting the configuration, the MaintenancePage is displayed.