-
Notifications
You must be signed in to change notification settings - Fork 380
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
Extract Initializer #757
Extract Initializer #757
Conversation
}, | ||
}; | ||
const defaults = { | ||
// TODO: bug that defaultEvents is not used? |
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.
@joellanciaux unclear from history, but I'm pretty sure you created defaultEvents
but it was never used. Thoughts?
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.
Yikes. I'm having a hard time following what was happening going off the blame
. 😅
I think it was related to some composition updates I was wanting at the time, but I'm positive it's a perfect candidate for removal.
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.
✅ 764a47e
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 have one comment regarding the removal of pageProperties
+ sortProperties
prop values, but this is huge!
I am seeing some issues on the TS front in storybook that I'm having a hard time tracking down. I might need to lean on @ryanlanciaux to investigate that bit. Are you seeing anything?
src/index.js
Outdated
const mergedStyleConfig = _.merge({}, defaultStyleConfig, ...plugins.map(p => p.styleConfig), styleConfig); | ||
|
||
const pageProperties = Object.assign({}, { | ||
pageProperties: { |
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.
It looks like the pageProperties
and sortProperties
properties were removed. Can this be re-added?
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 don't recall any issues from a month ago when I was working on this, but I can take another look. I take it you approve of this direction? If so, I'll rebase on |
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 have rebase
d on master
and brought part of the listeners
setup into initializer
.
It seems like we could refactor StoreListener
a bit to get the rest of the initialization out of the constructor (and more testable), but that's a more substantial change than the rest here.
I've left two notes related to spots where the general defaults < plugins < user props precedence is currently violated. Fixing them would technically be a breaking change, but I'm all about the principle of least surprise.
|
||
assert.deepEqual(ctx.events, { | ||
Plugin: 1, | ||
User: false, // TODO: bug that plugins overwrite user events? |
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.
@joellanciaux @ryanlanciaux is it intentional that events
provided in <Griddle events={...}>
are overwritten by events
from plugins?
|
||
assert.false('defaults' in ctx.listeners); | ||
assert.deepEqual(ctx.listeners.plugin(), 1); | ||
assert.deepEqual(ctx.listeners.user(), false); // TODO: bug that plugins overwrite user listeners? |
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.
@joellanciaux @ryanlanciaux again: should <Griddle listeners={...}>
be overwritten by plugins?
cc @shorja
Inspired by #760, I've added commits to pull the current I did not bring in @shorja's |
@dahlbyk Yeah now that I look at the core plugin taking in props, that's probably not a great way to handle stuff, especially since the pattern seems to be passing in init config to a plugin is done outside griddle -> . So really anything that should go into core really should be passed in like that, props should have nothing to do with it. Is the idea of plugins as reducers to basically chain the output of built plugins into the next plugin to build and then each one can do what it wants to wrap components? I've also been thinking about refactoring the component composition logic, included in my mega PR was a change to allow enhancers to 'stack' but I was planning on revisiting it. I drew up a diagram that explains the behaviour I was thinking about. |
Yeah, given a series of function plugins, you can think of I like the idea of being able to apply multiple enhancers, as your diagram explains, but explaining "enhancers cleared by a new component" rules might be less intuitive than making (function) plugins decide how old/new enhancers should compose (c.f. #733 (comment)). I'd suggest the plugin composition conversation continue over there. |
I like this direction and definitely wonder if pushing the enhancer registration logic to the plugins themselves would make the logic easier to understand for anyone using the library. |
@joellanciaux are you still seeing this? Thoughts on the |
I'm in favor of moving this one forward. If there are no storybook + typescript issues (I'm not seeing any atm) I will merge this. Sound good? :D |
Griddle major version
1.x
Changes proposed in this pull request
Pulls the default+plugin+props composition out of the Griddle constructor into a testable initializer.
This is really just prep to work on supporting function plugins for #733.
Why these changes are made
Tests are good, especially for this sort of composition (which seems likely to get more complex).
Are there tests?
Lots!