Skip to content

Expose fastbootConfigTree hook#515

Merged
kratiahuja merged 1 commit intoember-fastboot:masterfrom
kratiahuja:config-tree
Oct 2, 2017
Merged

Expose fastbootConfigTree hook#515
kratiahuja merged 1 commit intoember-fastboot:masterfrom
kratiahuja:config-tree

Conversation

@kratiahuja
Copy link
Contributor

@kratiahuja kratiahuja commented Aug 27, 2017

Fixes #513 . This unblocks engines support in FastBoot

Exposes fastBootConfig tree hook that addons can use to write additional config to expose in Node. In addition also bumped the schema version since the config schema is changed a bit.

TODO:

cc: @rwjblue

@kratiahuja
Copy link
Contributor Author

Tests will pass once new version of fastboot is published and updated.

README.md Outdated
fastbootConfigTree() {
return {
'<engine-name>/config/environment': {
'foo': 'bar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing an end quote here (throws off the markdown parser)

contents.push(
'if (typeof FastBoot !== \'undefined\') {',
'return FastBoot.config();',
'return FastBoot.config(\'' + appConfigModule + '\');',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this doesn't have to change right (since we do not intend to require the argument)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't have to change. Just wanted to make it consistent and less magic :)

@@ -0,0 +1,11312 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should stick with either yarn.lock or package-lock.json in this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I didn't mean to commit package-lock.json.

// this advance hook.
this.project.addons.forEach((addon) => {
if (addon.fastbootConfigTree) {
let configFromAddon = addon.fastbootConfigTree();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kratiahuja should environment be passed to this hook as an argument? Similar to how environment is provided to ember-cli Addon config hook https://ember-cli.com/api/classes/Addon.html#method_config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't see that is something going to be used by addons unless you have a usecase in mind.

@kratiahuja
Copy link
Contributor Author

Ok so all tests on Node 6 and Nod 8 should pass now. Node 4 will pass once ember-data issue is merged.

@rwjblue this should be ready for another review.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one minor fix in the README, but otherwise looks good to me.

README.md Outdated
```js
fastbootConfigTree() {
return {
'<engine-name>/config/environment': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the key here now should be <engine-name>.

@kratiahuja
Copy link
Contributor Author

Node 4 is breaking again on ember data.

@kratiahuja
Copy link
Contributor Author

Restarted Node4 travis job. If it passes, I'll merge and release.

@kratiahuja
Copy link
Contributor Author

Thank you @SergeAstapov for the help here!

@kratiahuja kratiahuja merged commit 4cbff43 into ember-fastboot:master Oct 2, 2017
@kratiahuja kratiahuja deleted the config-tree branch October 2, 2017 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants