search

trailsjs / trails

:evergreen_tree: Modern Web Application Framework for Node.js.
http://trailsjs.io
Other
1.67k stars 70 forks source link

v3 config merge fix #317

Closed jaumard closed 6 years ago

jaumard commented 6 years ago

Description

Config between app/trailpack is merge in the wrong way.

No fix yet, just create the right unit test of what we really want. Waiting for @tjwebb feedback on #316 issue

Issues

jaumard commented 6 years ago

Now merge is done in the correct order, but arrays are still not merge, just replaced :/ not ideal the problem is in some cases we want arrays to be replaced because it's just for a default value, on some other cases like routes, policies we want them merged we should try to find a solution for this

jaumard commented 6 years ago

one idea we can do to fix this is to have a hook arrayMergeStrategy or whatever and under trailpacks constructor we call this hook to let know trails what strategy use to deal with arrays. Something like: arrayMergeStrategy('policies', MergeStrategy.REPLACE) or arrayMergeStrategy('footprints.controllers.ignore', MergeStrategy.MERGE) What do you think @trailsjs/maintainers ?

jaumard commented 6 years ago

ping @trailsjs/maintainers

scott-wyatt commented 6 years ago

Honestly, I've been dealing with quite a few arrays lately and have found them so much slower in this context and like you said, impossible to merge correctly. I figure in configurations, we should just eliminate array usage :-p

jaumard commented 6 years ago

I agree with you but it will be some work to do that as we use arrays for routes, policies, footprints, trailpacks... and we need to find other way than arrays to do those configurations...

For now I put it place the previous strategy of the v2 who was to do basic merge between arrays with simple values. Can we merge and deploy this ?

tjwebb commented 6 years ago

basic merge between arrays with simple values.

It's problematic to merge arrays differently depending on what types they contain. Merging arrays reliably isn't possible.

jaumard commented 6 years ago

@tjwebb so basically we should go to no arrays in config, for now I put back the same behavior as v2 because it's mandatory for at least current trailpacks to be compatible with v3.

What is the next step ? Merging this as it is or first revert support for arrays and update all trailpacks to remove arrays in config files ?

tjwebb commented 6 years ago

trails v2 doesn't really "support" array merging. We can use arrays in config, but we can't rely on them being merged the way we expect.

https://github.com/trailsjs/trails/pull/317/commits/268fa60ed2c0595b38cacaeecc91cf6218d4ad8c#diff-76e2d0b77344d5df498c96cffed2204dR49

This looks like it concatenates arrays -- what if there are duplicate items afterwards? If a trailpack wanted to overwrite an existing array, that would be impossible with this code, right? So I'm saying it's not really a matter of whether it's "supported" or not, it's just a question of how the merging occurs. I think overwriting is actually a better and safer approach.

To me, concatenating arrays like this is equivalent to setting a value 3, and a trailpack sets the value 4, and instead of the value ending up as 4, they are "merged" and then the value 7 actually results. It's debatable whether that's a feature or a bug...

jaumard commented 6 years ago

@tjwebb I agree v2 didn't fully support merging array that's why I say I put back the same behavior here. Because for now we using arrays like this:

trailpack put MyController: ['MyPolicy.method'] and app put MyController: ['MyAppPolicy.method'] and in v2 it will end up like MyController: ['MyPolicy.method', 'MyAppPolicy.method'] but now in v3 we'll get MyController: 'MyAppPolicy.method'.

I'm fine with this but it's a big breaking change and trailpacks need to updated accordingly and not rely on this anymore.

I'll revert the array support like this we can merge the fix

jaumard commented 6 years ago

@tjwebb I revert the array stuff, just notice that my changes are breaking the trailpack-hapi because of this kind of usage https://github.com/trailsjs/trailpack-hapi/blob/master/lib/server.js#L32 should use the config.get now and not directly call fields. Any feedbacks ?

matteozambon89 commented 6 years ago

@jaumard seems like if I override trails package with git+https://github.com/trailsjs/trails.git#bugfix/v3_fixes, trailpack-winston will fail on constructor because this.app and app both return undefined instead of Trails.JS App.

jaumard commented 6 years ago

@matteozambon89 it not app that is undefined, but app.config because I merge it after trailpacks instantiation to fix this bug, and trailpack-winston need the all log config (merged) on his constructor which is wrong because not possible to have. @tjwebb any idea on doing trailpack-winston initialization differently ? Can we add an event trails:configured that trailpack-winston will listen to do his stuff ?

scott-wyatt commented 6 years ago

@jaumard is this a "chicken/egg" situation where we need to merge this so that we can update other libraries so that this one passes tests?

jaumard commented 6 years ago

Yeah kind off but if we merge this, there no way to make trailpack-winston work after, we first need to find a way for it to be able to provide logger to trails, trailpack-winston is not really a trailpack as it need to provide the logger before any trailpack can be instantiate and for now it can't

jaumard commented 6 years ago

So I add a trails:configured event here, I also test that it actually fix trailpack-winston, I have a local branch on it and it works with this fix. I can't create a PR a trailpack-winston yet because I don't have the permissions to push a branch on the repo :(

I just check the test and there two trailpacks who fails, winston which I fix already and hapi. Can anyone on @trailsjs/maintainers using hapi have a look ?