Skip to content
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.

Sign up for GitHub

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

Added feature for override trailpacks configs from the application config #39

Closed
wants to merge 4 commits into from
Closed

Added feature for override trailpacks configs from the application config #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
233b3aa
Added feature for override trailpacks configs from the application co…
catrielmuller Sep 20, 2016
50d3d13
Fix Test
catrielmuller Sep 29, 2016
b0ec4e1
Validate the basic Configuration
catrielmuller Oct 11, 2016
84fbd7d
Fix Err
catrielmuller Dec 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ module.exports = class Trailpack {
if (!pack.config) {
pack.config = { }
}
if (!pack.config.trailpack) {
Copy link
Member Author

@catrielmuller catrielmuller Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@jaumard
About the issue #1
If you dont pass a configuration for the trailpack, create a empty object

Copy link
Member Author

Choose a reason for hiding this comment

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

And here https://github.com/catrielmuller/trailpack/blob/50d3d13cb4b03fc0f5cd56e5c303fb5faf15fe20/index.js#L45 this empry object recive the default configurations for the trailpack.
I think we dont need a error message if the Trailpack use the base config if dont exist

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need :) in case user fill the config with wrong values we need to check config is fill with correct types or throw an error

Copy link
Member Author

Choose a reason for hiding this comment

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

@jaumard I dont undertand, If the trailpack don't have any required configuration will be use the default config from the merge. The only proble if the module expect a event and this dont exist or not be defined by another trailpack, but in this case we cant manage this problem cos is a responsability of the developer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@catrielmuller what if the user set this as a trailpack config:

lifecycle: {
         listen: ['trailpack:waterline:initialized']       
      }

Or


lifecycle: {
        initialize: ['trailpack:waterline:initialized']
      }

Configurations are wrong but not empty, with a JOI schema we can check the entry like we do on trails/trailpack to check other config files. A users have an error message clear on what's going not.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jaumard Take a look of the last commit, something like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can make the validation with joi, if is not a problem add joi as dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem to add joy as dependency as almost all trails module have it. It will made it less verbose I think but it's the idea yes :)

pack.config.trailpack = { }
}

Object.defineProperties(this, {
app: {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"eslint": "^2.11.1",
"eslint-config-trails": "^1.0.7",
"mocha": "^2.5.0",
"trails": "^1.0.3",
"trails": "catrielmuller/trails#config-trailpacks-in-app",
"smokesignals": "^1.2.2"
},
"engines": {
Expand Down
8 changes: 6 additions & 2 deletions test/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,12 @@ const App = {
},
main: {
packsConfig: {
'trailpack-test': {
testattr: 'testvalue'
'trailpack-testpack': {
lifecycle: {
initialize: {
emit: ['trailpack:testpack:custom']
}
}
}
},
packs: [
Expand Down
23 changes: 8 additions & 15 deletions test/pack/config/trailpack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,27 @@
* @see {@link http://trailsjs.io/doc/trailpack/config
*/
module.exports = {

type: 'misc',
/**
* API and config resources provided by this Trailpack.
* Configure the lifecycle of this pack; that is, how it boots up, and which
* order it loads relative to other trailpacks.
*/
provides: {
api: {
controllers: [ ]
// ...
},
config: [ ]
},

events: {
lifecycle: {
configure: {
/**
* List of events that must be fired before the configure lifecycle
* method is invoked on this Trailpack
*/
listen: [ ],
listen: [],

/**
* List of events emitted by the configure lifecycle method
*/
emit: [ ]
emit: []
},
initialize: {
listen: [ ],
emit: [ ]
listen: [],
emit: []
}
}
}
6 changes: 2 additions & 4 deletions test/pack/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
'use strict'

const app = {
config: require('./config')
}
const Trailpack = require('../../')

module.exports = class TestPack extends Trailpack {

constructor () {
constructor (app) {
super(app, {
config: require('./config'),
pkg: require('./package')
})
}

}
4 changes: 3 additions & 1 deletion test/trailpack.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ describe('Trailpack', () => {
new TestPack(global.app)
})
it('should override the trailpack config', () => {
global.app.after('trailpack:testpack:constructed').then(() => {
assert.equal(pack.config.lifecycle.initialize.emit[0], 'trailpack:testpack:custom')
})
const pack = new TestPack(global.app)
assert.equal(pack.config.trailpack.testattr, 'testvalue')
})
})

Expand Down