-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Render Element Modifiers #415
Conversation
## Drawbacks | ||
|
||
* Element modifiers are a new concept that haven't been fully stabilized as of | ||
yet. It may be premature to add default modifiers to the framework. |
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.
FWIW, I think the only way to make them "main stream" is to motivate their use cases which I think this RFC does very well!
@pzuraq - I for one, am very excited to move forward here! These APIs would be extremely helpful for users of sparkles-component (conceptually similar to tagless components) and even certain simple needs of template only glimmer component users... |
Co-Authored-By: pzuraq <me@pzuraq.com>
I like this a lot! The one thing that I have pretty mixed feelings about, though, is The primary benefits that I see of this RFC are being able to run setup/teardown code for elements. The ability to interact with elements in between those events without using I'm also really concerned about breaking symmetry with I also think it's counter-intuitive to call |
@bendemboski that's great feedback! I agree that breaking symmetry feels a little wrong at first, and I definitely agree that we have not motivated it well in this RFC in isolation. I think the motivations can be extracted from the Glimmer components RFC. For instance, one common use case is to setup or teardown a plugin whenever the component is We felt it was best to allow users to handle these types of cases using modifiers, instead of the other proposed hooks (like Originally we debated whether or not we could have 3 modifiers, something like |
I have similar feelings about {{did-render}} breaking the symmetry with {{will-destroy}} as @bendemboski. I understand it is meant to behave like current didRender() hook in Component but I am afraid people will mix it easily with didInsertElement() and then wonder why things happen multiple times. |
It is key to remember that users will have to opt in to triggering the function multiple times by passing an argument. This will never rerun the action passed in: <div {{did-render (action this.setupElement)}}></div> While this will, if <div {{did-render (action this.setupElement) @disabled}}></div> I do think that splitting them out reads well too though: <div
{{did-insert (action this.setupPlugin)}}
{{did-update (action this.updatePlugin) @disabled}}
{{will-destroy (action this.teardownPlugin)}}
>
</div> |
Okay, I some how missed the opt-in part. But it is like having a method with boolean argument which changes its behavior ever so slightly. While for experienced Ember.js users this will be easily understandable that property mutation will cause a re-render then for new users it still might be confusing to crasp what is going. I am pretty sure that with passed in second argument anti patterns are a risk, like having boolean state values “if isFirstRender else” to save few lines in template of setting up 2 {{did-render}} modifiers - one with action for the didInsertElement behaviour and other one for update path with additional argument. |
I don't see this as covering those cases more cleanly. The functionality that it's covering (in the example component you linked) is "dynamically setup/teardown the hammer library anytime the disabled argument changes." But there's any number of reasons I might want to interact with an element in my DOM, and a change to an argument or property of my component is only one reason. For example, a service might trigger an I see making I think it makes much more sense to encourage people to use I also worry about nested properties: <div {{did-render this.doSomething itemData.type}}> will It seems like this is providing functionality that can already be achieved with existing primitives, and in doing so, making those primitives harder to understand and use. |
In the general case of reacting to a callback of some kind, or an async event, it would make sense to capture a reference to the element, definitely. However, in the more common and specific case of wanting to run the change when a specific arg/tracked-property/computed-property changes, I disagree. In fact, currently there are cases where observers are the only way for us to reasonably react to these changes: export default Component.extend({
// arguments
model: null,
didInsertElement() {
if (this.model.isEnabled) {
this._setupPlugin();
}
},
willDestroyElement() {
if (this.model.isEnabled) {
this._teardownPlugin();
}
},
updatePlugin: observer('model.isEnabled', function() {
if (this.model.isEnabled) {
this._setupPlugin();
} else {
this._teardownPlugin();
}
})
}); We have no guarantee that
This is a fair point too, but either way the validation logic will have to exist somewhere. The same issue would exist using hooks: export default Component.extend({
didUpdateAttrs() {
if (this.itemData.type !== this._previousType) {
this._previousType = this.itemData.type;
this._updatePlugin();
}
}
}) And this actually compounds the problem, since the hook is disconnected from the action we are trying to take. You can see in many examples that these hooks tend to become a catch all for many different, semi-related or unrelated actions that all need to occur at once. Edit: Put another way, the current solutions through hooks often rely on timing and implicit state (that values are being consumed by templates). There are workarounds to these pain points, but allowing some modifiers to be passed argument values eliminates them entirely. |
Agreed! So if I have component code that needs to react to such changes, but doesn't happen to require direct DOM access, then I'm using observers anyway (or implementing a different notification mechanism like
Right, but that logic would be clear, readable, understandable, and not happening in hidden magical code that could end up doing something I don't want it to do. It also would be able to cover all use cases, rather than requiring a totally different implementation if I need to access one DOM element vs. two DOM elements with no common ancestor element vs. zero DOM elements.
@did-update-args
doThing1() {
// ...
}
@did-update-args
doThing2() {
// ...
} I understand the desire to be able to run code that interacts with DOM elements when some argument/state updates, but I don't think I've heard a convincing reason for why that has to be overloading My alternate proposals would be:
<div
{{did-render this.setSomeStuffUp}}
{{did-update this.changeSomeStuff @arg1 @arg2}}
/>
<div {{did-render this.doSomeStuff 'foo' @arg1 rerun-on=(array @arg1 @arg2)}}/> or maybe <div {{did-render (action this.doSomeStuff 'foo' @arg1) (rerun-on @arg1 @arg2)}}/> |
This is true, but the point is we're chipping away at the need for observers or external hooks by making dependencies more explicit in general. Like I said, I do think there will always be cases where we need to do something asynchronously and imperatively in JS apps, but the majority of cases do actually come down to field access.
The main point is to avoid having to use render hooks such as Option 2 also satisfies this constraint, since it would give users a way to trigger code to update their element plugins/code when arguments/tracked properties/computeds change. I personally have no objection to having three modifiers instead of two, and I do think it'll be clearer for some use cases. |
Okay, I think I have fully expressed (to say the least!) my concerns here, and I feel heard, so thanks for taking the time to have this discussion 😺 |
@bendemboski and I discussed this a bit more on Discord and agreed that there are definitely use cases for a |
@bendemboski Your arguments sparked a lot of good conversation on the core team. Thank you for taking the time to write them up! |
d4e0b91
to
af106cd
Compare
Will the destroy modifier be awaited? My main concern is the possibility of needing to do something async on destroy where the component is destroyed before the cleanup function finishes. |
33e51a0
to
993ffbb
Compare
I'm not sure if it belongs in this RFC or if it's more of an implementation detail, but does the |
993ffbb
to
e80bdd1
Compare
@NullVoxPopuli currently destruction hooks in components do not block on async, and I don't think it would be a good idea to do that here. You should be able to clean up asynchronously without blocking rendering in most cases, which I think is probably a better idea performance-wise. @bendemboski that's a great point, I do think that's more implementation specific but we should definitely consider it when we implement. My gut says yes? In fact it could already be handled by the double render assertion as is |
We reviewed this RFC at the core team meeting today, and believe that this is ready for final comment period. |
Co-Authored-By: pzuraq <me@pzuraq.com>
This has been in final comment period for around 10 days. There have been no new issues/concerns raised during FCP so this is ready to launch 🚀 . |
This is the initial implementation of [emberjs/rfcs#415](https://emberjs.github.io/rfcs/0415-render-element-modifiers.html). It currently depends on [ember-modifier-manager-polyfill](https://github.com/rwjblue/ember-modifier-manager-polyfill) in order to support Ember versions prior to 3.8 (should work back to at least 2.12).
Initial implementation of emberjs/rfcs#415
FYI - I just published @ember/render-modifiers 1.0.0 with support back to Ember 2.12 (by way of ember-modifier-manager-polyfill). There is still a bit of work to do (need tons more documentation), but its a good start... |
Begin fleshing out README with info from emberjs/rfcs#415.
Rendered
Huge thanks to @rwjblue, @krisselden, and others on this one!