Added host and protocol config fields for frontend and server packages #78
Conversation
Shouldn't make it look like constants
…er-platform into more-configuration
…er-platform into more-configuration
PatrickShaw
requested review from
calebj0seph,
TimGebert,
darvid7 and
hferrier
packages/frontend/src/build.ts
Outdated
| @@ -1,19 +1,21 @@ | |||
| import webpack from 'webpack'; | |||
|
|
|||
| import { readConfig } from '@openapi-platform/config'; | |||
| import { uiUrl } from '@openapi-platform/config'; | |||
There was a problem hiding this comment.
This is unused (are you running tsc to check?).
There was a problem hiding this comment.
Doesn't seem to pick up on it. We're using gulp-typescript but running tsc directly doesn't seem to pick up on it either. I'll look into it before the PR gets accepted.
There was a problem hiding this comment.
Figured it out. Seems we're going to have to deal with no-unused-variable deprecation warnings for a while: palantir/tslint#4100
packages/config/src/index.ts
Outdated
| return urlFromConfig(config, 'server'); | ||
| } | ||
|
|
||
| export function uiUrl(config): string { |
scripts/initDb.js
Outdated
| @@ -32,7 +32,7 @@ async function insertSampleData(client) { | |||
| async function run() { | |||
| const config = readConfig(); | |||
| const { client, socket } = createServerClient( | |||
| `http://localhost:${config.get('server.port')}`, | |||
| serverUrl(config), | |||
There was a problem hiding this comment.
This isn't going to work, what if my server address is 0.0.0.0? I'd probably add something like apiBaseUrl for the scripts.
There was a problem hiding this comment.
Yeah good pick up. It can just use ui.apiBaseUrl. I'm not adding extra config just for some dev scripts.
…er-platform into more-configuration
Bit of a hack but it works
Ignoring deprecation rule - It's still useful for autofixing
Gulpfile.js
Outdated
| const openapiPlatformConfig = readConfig(); | ||
| const createWebpackConfig = require(join(packageDir, 'webpack.config')); | ||
| const webpackConfig = createWebpackConfig({ | ||
| NODE_ENV: process.env.NODE_ENV, | ||
| API_PORT: openapiPlatformConfig.get('server.port'), | ||
| env: process.env.NODE_ENV, |
There was a problem hiding this comment.
Could you explain why this is needed when convict will automatically populate the env configuration option from NODE_ENV for us? Can't we just pass in config.env?
There was a problem hiding this comment.
You're so right, that didn't even occur to me. Good pick up :)
packages/config/src/index.ts
Outdated
| )}`; | ||
| } | ||
|
|
||
| export function uiUrl(config): string { |
There was a problem hiding this comment.
Neither of uiUrl or serverUrl are used anywhere. Can we remove these functions?
There was a problem hiding this comment.
serverUrl is the default to apiBaseUrl.
With regard to uiUrl I want to be consistent - If I provide url helpers for server and api urls, i want to do the same thing for the UI URL even if it's not used by anything at the moment.
There was a problem hiding this comment.
Fair point with uiUrl, but apiBaseUrl doesn't use serverUrl at all here.
Added some more configuration options to the mix. Not all of them are used properly just yet (E.g.
httpsfor the protocol field).