Extend FastBoot.config to read other provided config as well.#164
Extend FastBoot.config to read other provided config as well.#164
Conversation
kratiahuja
force-pushed
the
config-tree
branch
from
4041307 to
b4e3a05
Compare
rwjblue
left a comment
rwjblue
left a comment
There was a problem hiding this comment.
Overall, looks very good! Left a few inline notes/questions...
| let appConfigKey = `${this.appName}/config/environment`; | ||
| if (!appConfig.hasOwnProperty(appConfigKey)) { | ||
| this.config[appConfigKey] = appConfig; | ||
| } |
There was a problem hiding this comment.
We should probably create another environment variable to allow the entire config to be replaced, thoughts?
There was a problem hiding this comment.
What's the use case for that?
There was a problem hiding this comment.
Well, the same as the use case for process.env.APP_CONFIG in the first place. To allow customizing the configuration without requiring a recompile.
There was a problem hiding this comment.
Oho yes, I agree. Will add that.
src/ember-app.js
Outdated
| const config = this.config; | ||
| const appName = this.appName; | ||
| function fastbootConfig(key) { | ||
| if (!key && config) { |
There was a problem hiding this comment.
I don't think this guard is needed, defaulting the key here when not provided to appname/config/environment (regardless if config is available) seems totally fine, no?
There was a problem hiding this comment.
Yeah I might have forgotten to clean this up..
src/ember-app.js
Outdated
| if (process.env.APP_CONFIG) { | ||
| this.appConfig = JSON.parse(process.env.APP_CONFIG); | ||
| let appConfig = JSON.parse(process.env.APP_CONFIG); | ||
| let appConfigKey = `${this.appName}/config/environment`; |
There was a problem hiding this comment.
Should the keys be ${appName}/config/environment or just appName? I'd generally prefer the config keys to be "opaque" (so either is fine), but I'd lean towards making the key in this object just appName here (though I don't feel super strongly).
There was a problem hiding this comment.
I am fine with anything. The reason I went with appName/config/environment since it is easier to map to the AMD module name.
src/fastboot-schema-versions.js
Outdated
| 'base': 1, // first schema version supported by fastboot library | ||
| 'manifestFileArrays': 2 // schema version when app and vendor in manifest supported an array of files | ||
| 'manifestFileArrays': 2, // schema version when app and vendor in manifest supported an array of files | ||
| 'configExtension': 3 // schema version when FastBoot.config can read artibitary indexed config |
There was a problem hiding this comment.
typo here, artibitary -> arbitrary
|
@kratiahuja - Ping? |
kratiahuja
force-pushed
the
config-tree
branch
from
b4e3a05 to
8ccdd6d
Compare
kratiahuja
force-pushed
the
config-tree
branch
from
8ccdd6d to
fb8e4b5
Compare
|
@rwjblue updated per feedback. Super sorry for the delay here! |
2 participants
Fixes #163 . This will unblock engines support in FastBoot.
Motivation
Today by default FastBoot provides the ability to read config based info in Node for host applications only. Other infrastructure like Ember Engines may provide their own configuration per bundle. In order to support this, we want to extend
FastBoot.configto have akeyas an input which determines which config to return a JSON blob.Things done in this PR
fastbootto 3 since the format has changedFastBoot addon
Corresponding FastBoot addon PR: ember-fastboot/ember-cli-fastboot#515
cc: @rwjblue