Feature/middleware setup - Basic Architecture and API to fetch Dashboard Configuration#367
Feature/middleware setup - Basic Architecture and API to fetch Dashboard Configuration#367
Conversation
curran
left a comment
curran
left a comment
There was a problem hiding this comment.
Very nice! Looks good in terms of choices for dependencies and build.
| var client = null; | ||
| let config = function () { | ||
| return { | ||
| host: process.env.APP_ELASTICSEARCH_HOST ? process.env.APP_ELASTICSEARCH_HOST : null, |
There was a problem hiding this comment.
currentConfig.host = state.ES.get(ActionKeyStore.ES_HOST) || process.env.REACT_APP_ELASTICSEARCH_HOST;
We are missing to parse the first param, arnt we?
src/services/configurations/index.js
Outdated
|
|
||
| const config = { | ||
| path: process.env.PUBLIC_URL + "/configurations/", | ||
| api: process.env.REACT_APP_API_URL, |
There was a problem hiding this comment.
Can we please have a config.js file where all the configs are assigned. Accessing process.env directly is not a good idea. Also, have a default value set for each one of them
There was a problem hiding this comment.
I think its fine to have process.env.REACT_APP_API_URL, as we build the app for production, then this will going to be replaced.
| release: require('../../package.json').version, | ||
| baseDir: path.normalize(__dirname + '/../..'), | ||
|
|
||
| apiPrefix: '/api', |
There was a problem hiding this comment.
shall this be made /reports/api ?
|
As per our discussion: Advantages of it is that whenever we need to update some config then we only have to update the config file instead of rebuilding the project. Please suggest. |
…iddleware-testing
…iddleware-testing
nxanil
force-pushed
the
feature/middleware-setup
branch
from
4336144 to
296069c
Compare
4 participants
@ronakmshah @curran
Please review the setup of Express JS middleware and api to fetch default configurations.
Setup instruction are updated into Readme of root folder as well as server folder.