-
Notifications
You must be signed in to change notification settings - Fork 400
Refactor configuration for server and client #280
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
Conversation
bin/webpack-dev-server.js
Outdated
const webpackDevMiddleware = require('webpack-dev-middleware'); | ||
const webpackHotMiddleware = require('webpack-hot-middleware'); | ||
const webpackDevConfig = require('config/webpack.dev.config.babel').default; | ||
const webpackDevConfig = require('../webpack.dev.config.babel').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.
This should probably work without ../
following recent changes.
webpack.prod.config.babel.js
Outdated
import SriStatsPlugin from 'sri-stats-webpack-plugin'; | ||
|
||
import config from './index'; | ||
const config = require('config'); |
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 could be an import, I'll clean that up in due course.
bd61c07
to
558ffd9
Compare
@@ -0,0 +1,79 @@ | |||
// CONFIG defaults (aka PRODUCTION) |
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.
On other projects we've made development the default. I guess since this is still no-configuration-needed for development having production default is cool?
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.
Yeah, I feel it's a better way around to have production config happen if you don't override something than to have development config if you don't override something.
module.exports = { | ||
serverPort: 3000, | ||
|
||
apiHost: 'https://addons-dev.allizom.org', |
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 updated this to:
apiHost: process.env.API_HOST || 'https://addons-dev.allizom.org',
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.
Oh that needs to be done differently to use the proper node-config way. I'll update #298.
Fixes mozilla/addons#9569
Fixes mozilla/addons#9565
Fixes mozilla/addons#9559
/src
../
etc@jasonthomas this will need some modifcations to the deployment since you'll need to specify
NODE_ENV
in the deploy scripts after this lands.e.g:
Replacing
dev
withstage
orproduction
for the other cases anddisco
withsearch
for the search app deployment.