-
Notifications
You must be signed in to change notification settings - Fork 781
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
Allow mixins to export a view and remove render event. #382
Comments
I like this change are there any circumstances where we would still want the render event though? I guess with this it's not needed now, the only mixin I have that has been using it was my router. |
Ah, yes, I forgot to mention that if we introduce this proposal, I'd like to remove the
Good question. Would you be able to rewrite your mixin to take advantage of this proposal? /cc @zaceno @SkaterDad |
@jorgebucaran oh ya I can adapt my router, I'm just asking mainly out of curiosity if there are other use-cases where it couldn't be replicated with the view exported from mixin. I can't think of a use myself outside of routing. |
We discussed this on Slack. The summary is I think,
|
The render event is a nice place to make pre-wired components (i.e. the opposite of what @rajaraodv said, and which I am very keen to continue doing). It's possible to do it without the render event of course, but it requires mixins to define their own custom events. So that's a tiny argument in favor of keeping the render event. But as long as we keep custom events, so there is some way to access the current state and actions outside of actions and the view, I'm happy. |
@rajaraodv I don't recall the specific slack convo that resulted in preference for passing mixin state & actions as props. But I know feelings are still mixed on that. For instance this issue: #365
|
@zaceno Why would you need the render event to pass pre-wired views if the proposal in this issue is implemented? @jorgebucaran I really like this idea. Is it fair to assume the components would be pre-wired only a single time on app startup? The other way, injecting components in the render event, seems less efficient, since you have to inject them on every update. |
@SkaterDad I don't strictly need the const myMixin = emit => {
const prewired = fn => (props, children) => emit('wireMeUp', (state, actions) => fn(state, actions, props, children)
return {
events: {
wireMeUp: (state, actions, fn) => fn(state, actions)
},
view: {
mymixinNamespace: {
CompA: prewired((state, actions, props, children) => <div> ....</div>),
CompB: prewired((state, actions, props, children) => <div> ... </div>),
}
}
} ... the only problem with that is that if multiple mixins want to use this pattern, they'll each have to register their own The render event is a convenient, one-stop-shop for doing prewiring, as it stands today. |
@zaceno But isn't the proposal in this issue supposed to build that pre-wiring functionality into how mixins are initialized? Maybe I'm confused. ❓ |
@zaceno This was from here: https://hyperapp.slack.com/archives/C41ECC0V6/p1505964096000002 |
@SkaterDad I don't think @jorgebucaran is proposing that Hyperapp does prewiring for you. Not in this RFC anyway. Or, at least, not how I understand the term (but I may have misunderstood it). By prewiring, I mean making state and actions available to a component, so that the user of a mixin doesn't have to pass state and/or actions as props. This proposal, as I understand it, is just about letting mixins define components in the view prop and have them injected. I don't have a problem with it, From @jorgebucaran 's perspective (correct me if I'm wrong) it's mainly about trying to get rid of the render event |
@zaceno His example specifically mentions prewiring actions to a component, as an alternative to passing a 3rd arg to the view during the render event. The router's will be a good use case for it. Hello(props, children) {
// This is a regular component.
// What's special about it is that's inside the view closure, so
// it's effectively "pre-wired" to the state and actions.
return (
<a
style={{ color: state.hello.color }}
href={props.url}
onclick={actions.hello.doSomething}
>
{children}
</a>
) I don't use the render event for anything at this time, so I'm neutral on removing it. However, it hardly takes up any bytes to keep it around, so what's the harm? |
@SkaterDad You're absolutely right! I completely missed that mixin.view was described as a function, rather than an object. LGTM! |
I'd like to move away from this idea. There is so much going on at the moment, that I don't have time to carefully explain, as I should, why I am not moving forward with this, but I will in the PR linked below. |
Mixins can extend your app's state, actions and events. Take it one step further and let them export a
view
function too.A mixin's view would work by returning an object of one or more components.
While the state, actions and events props are plain objects that we can merge for you, we can't merge views as they are functions! But we can collect the views into an object and give it to you.
This can be useful for components that work together with a mixin like the Router/Link.
This means instead of e.g.,
myLibrary.Hello
component andmyLibrary.hello
mixin, now you could do this instead.Then you can then use it like this.
Your comments and questions please. 👋
The text was updated successfully, but these errors were encountered: