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.

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

Config trailpacks in app #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

catrielmuller
Copy link
Member

Description

Need for implement this feature trailsjs/trailpack#39

Issues

-resolve trailsjs/trailpack#39

@scott-wyatt
Copy link
Member

This is nifty.

@scott-wyatt
Copy link
Member

@trailsjs/maintainers could we get some conversation going about this? This is pretty useful.

@jaumard
Copy link
Contributor

jaumard commented Oct 5, 2016

@scott-wyatt sound pretty good idea for me, @tjwebb what do you think of this ?

@scott-wyatt
Copy link
Member

@trailsjs/maintainers nudge nudge.

@scott-wyatt scott-wyatt mentioned this pull request Nov 21, 2016
7 tasks
@tjwebb
Copy link
Member

tjwebb commented Nov 21, 2016

Do we need a separate config namespace for this? For example, trailpack-knex is configured by adding stuff to config.database. Why not continue to custom-configure the trailpack there? Seems like we're adding an unnecessary branch to the config tree with "packConfig"

@jaumard
Copy link
Contributor

jaumard commented Nov 21, 2016

@tjwebb and how did you do for trailpacks who doesn't have a specific config file ? or have multiple...
But maybe instead of putting this into main we can just use a trailpacks.js

@tjwebb tjwebb modified the milestone: 2.1 Feature Release Nov 22, 2016
@tjwebb
Copy link
Member

tjwebb commented Dec 6, 2016

@catrielmuller @scott-wyatt @jaumard I thought about this some more, and unless I'm misunderstanding this PR still, I don't think this is needed. This functionality is already possible, and is quite easy. You don't need a special config file or namespace for this to work.

If a trailpack makes use of a config value app.config.foo.bar, that value can be overridden by simply configuring that value in the application. I'm not seeing what you aren't able to do.

also fwiw, I'm including lodash in trails v2.0 so we don't need to write so much merge logic ourselves.

@scott-wyatt
Copy link
Member

@tjwebb I think what you are saying is true and if there is an alternative that is easy and doesn't require new code in trails, then that should be the side to err on.

@jaumard
Copy link
Contributor

jaumard commented Dec 7, 2016

@tjwebb what we want to achieve here is to be able to modify the lifecycle event of trailpacks. I don't think it's currently possible or please let us know how :) because lifecycle are not in app.config if I'm right

@tjwebb
Copy link
Member

tjwebb commented Dec 7, 2016

what we want to achieve here is to be able to modify the lifecycle event of trailpacks

aha! Ok I understand now. So we're talking about problems like this:

Installed are packs A, B, and C. A's lifecycle is configured to wait for B. But C might be some other pack where it may want to alter the behavior of A by reconfiguring stuff, and so you'll want to modify A's lifecycle to wait for C as well. So while A doesn't depend on C, its behavior may change if C happens to be installed.

We may want to include this sort of feature in the trailpack API rather than relying on the developer to directly modify configuration. This is the first example API that comes to mind (feel free to critique if I still don't have something right):

// inside trailpackC.configure

const trailpackA = this.app.packs.A
trailpackA.modifyLifecycle('add', {
  stage: 'initialize',
  listens: [ 'trailpack:C:initialized', 'foobar' ]
})

In this example, the method would add the events trailpack:C:initialized and foo:bar to the initialize lifecycle stage in the configuration.

By running this action through a method, we'll be able to provide much better debugging info to the user when something isn't working, by being able to log exactly what lifecycle modifications are occurring among the trailpacks.

@jaumard
Copy link
Contributor

jaumard commented Dec 8, 2016

I like this solution ! I just wondering if user will need/want to use this in there project outside trailpack... But maybe I can update trailpack-bootstrap to have 4 methods validate, configure, initialize, afterStart, now we only have afterStart. Like this if user want to modify trailpack lifecycle they can too.

@tjwebb
Copy link
Member

tjwebb commented Dec 8, 2016

now we only have afterStart

When does afterStart run?

You can listen to the trails:ready event if you want to run some logic after all trailpacks are loaded, or trails:start which is right before all trailpacks are loaded

@jaumard
Copy link
Contributor

jaumard commented Dec 9, 2016

@tjwebb afterStart is not a good name it's onReady (https://github.com/trailsjs/trailpack-bootstrap/blob/master/index.js#L27) :)

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.

4 participants