-
Notifications
You must be signed in to change notification settings - Fork 6
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
Added feature for override trailpacks configs from the application config #39
Added feature for override trailpacks configs from the application config #39
Conversation
This feature need this PR too trailsjs/trails#236 |
This is actually a great feature. Some other good use cases is altering the order of event listeners amongst trailpacks which is a must for some of my custom packs. |
@catrielmuller there some problems with tests can you look ? |
@jaumard Im modify the package.json because i need this pr (trailsjs/trails#236) in trailpack for have the app.config.main.packsConfig defined. |
The last commit fix this issue #1 too |
@catrielmuller ho ok ! I understand ^^ we'll wait for the Trails PR first then :) |
@catrielmuller I didn't see where you fix #1 ? Did you push everything ? |
@@ -28,6 +28,9 @@ module.exports = class Trailpack { | |||
if (!pack.config) { | |||
pack.config = { } | |||
} | |||
if (!pack.config.trailpack) { |
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.
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.
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
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 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
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.
@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.
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.
@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.
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.
@jaumard Take a look of the last commit, something like this?
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 can make the validation with joi, if is not a problem add joi as dependency.
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.
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 :)
In some cases one trailpack run some functionalities from the application, for example.
If you implement a tasker trailpack for run tasks in background and this task need a orm, In the startup of the application if the tasker module are loaded before the orm module this fails cos app.orm is undefined.
But this not is a problem of the tasker trailpack cos this module dont need an orm really (If you use as standalone).
For solve this problem and another case like the order on startup app as you want.
I propose include a new attribute in the config/main.js "packsConfig" and inside of this you can override the trailpacks configs like this:
As well also you can override this attribute per enviroment.