-
-
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
Pre-RFC: render helpers (analogous to render modifiers) #484
Comments
I noticed a similar thing emerging with I do worry a little that such patterns encourage users back towards using observer-style patterns. although at least this time they're not synchronous! The For a component that cares about lifecycle but has no template I expect the recommendation is to use |
In the meantime I did manage to update Unfortunately, this pollutes the global namespace. However, once template imports come along, this won't be an issue any more. To me it still feels a bit weird to off-load all this to the template, but I am trying align my brain with the "the template is the driver of the program / state" mentality. Even when template imports land, I still think that |
Tangentially related: I made {{on this.someEventTarget "click" this.someListener}} I needed it in order to bind a global |
In my opinion, a // components/overlay.js
import Component from '@glimmer/component'
import { tracked } from '@glimmer/tracking'
import { action } from '@ember/object'
export default class OverlayComponent extends Component {
// @visible?: boolean
// @destinationElement?: HTMLElement
@tracked visible = this.args.visible
@action
didUpdateVisible () {
this.visible = this.args.visible
}
@action
close () {
this.visible = false
}
} {{! -- templates/components/overlay.js --}}
{{did-update this.didUpdateVisible @visible}}
{{#if this.visible}}
<EmberWormhole @destinationElement={{@destinationElement}}>
<div class="overlay-container">
<div class="overlay" ...attributes>
{{yield (hash
close=this.close
visible=this.visible
)}}
</div>
</div>
</EmberWormhole>
{{/if}} In the current programming model, what's the alternative? I tried using the following in the constructor, but that didn't track mutations: addObserver(this.args, 'visible', this.didUpdateVisible.bind(this)) |
I just published |
This would be a very welcome addition for renderless / provider components. E.g. you had to use an arbitrary, invisible element to which you bound a modifier that updates back to your component. So, elements are not needed anymore. before: <meta {{did-update this.updateFoo @foo}}> after: {{did-update this.updateFoo @foo}} I still think |
There are only very few cases where However, if you imagine a world where templates are not necessarily backed by a component class instance, they might become more useful. Either way, now we have full feature parity in both directions. Just because something is there, doesn't mean that you have to use it. And luckily, with embroider on the horizon, unused helpers and modifiers will be stripped from the build. 🙌 |
@willviles I think that pattern should be discouraged in general. That was actually a major reason why the lifecycle hooks were removed, and a major discussion point on the Glimmer Components RFC. Fundamentally, that pattern is better rewritten as a getter based on derived state, rather than manually, imperatively updating state based on observed changes.
It's also probably important to note, in general we did not expect the render modifiers to be the recommended path forward in the future. They were seen as a way to unblock users who are converting to Octane and who have lifecycle-hook heavy apps, an escape-hatch essentially. In the long run, we wanted to push users toward either deriving state using autotracking, or writing modifiers specifically designed for solving targeted use cases. I'd say the same thing about these helpers if we make them (and I definitely think we should! They're also a valuable escape-hatch).
I'm wondering if this is an indicator that the design of the escape hatches is off. There was an initial proposal for a |
Followup to @willviles example (which I misread in my previous comment, edited above). Let's say we have the following as our component today: // components/overlay.js
import Component from '@ember/component';
export default Component.extend({
visible: false,
destinationElement: null,
actions: {
close () {
this.set('visible', false)
}
}
} {{! -- templates/components/overlay.js --}}
{{#if this.visible}}
<EmberWormhole @destinationElement={{@destinationElement}}>
<div class="overlay-container">
<div class="overlay" ...attributes>
{{yield (hash
close=(action 'close')
visible=this.visible
)}}
</div>
</div>
</EmberWormhole>
{{/if}} {{!-- invocation --}}
<Overlay
@visible={{this.overlayVisible}}
@destinationElement={{this.overlayDestination}}
as |overlay|
>
Hello, world!
<button {{action overlay.close}}>
Close
</button>
</Overlay> This should generally work pretty well. When we initially toggle
When we first try to convert this to Glimmer Components, we run into an issue with the // components/overlay.js
import Component from '@glimmer/component';
import { action } from '@ember/object';
export default class Overlay extends Component {
@action
close () {
this.args.visible = false; // this will throw an error
}
} So, what do we do here? The issue is that we're using a single class field for state that is both internal to this component, and external to it, in the surrounding context. So, one option we could try is to make a getter that combines internal component state and external argument state: // components/overlay.js
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';
export default class Overlay extends Component {
@tracked _visible = true;
get visible() {
return this.args.visible && this._visible;
}
@action
close () {
this.visible = false;
}
} This will work at first! However, when we toggle the overlay off, we'll find that it's never possible to turn it back on. It's internal state is now frozen. This is what led to the // components/overlay.js
import Component from '@glimmer/component'
import { tracked } from '@glimmer/tracking'
import { action } from '@ember/object'
export default class OverlayComponent extends Component {
// @visible?: boolean
// @destinationElement?: HTMLElement
@tracked visible = this.args.visible
@action
didUpdateVisible () {
this.visible = this.args.visible
}
@action
close () {
this.visible = false
}
} {{! -- templates/components/overlay.js --}}
{{did-update this.didUpdateVisible @visible}}
{{#if this.visible}}
<EmberWormhole @destinationElement={{@destinationElement}}>
<div class="overlay-container">
<div class="overlay" ...attributes>
{{yield (hash
close=this.close
visible=this.visible
)}}
</div>
</div>
</EmberWormhole>
{{/if}} This works, but it definitely feels a bit strange. We have an action that is called from the template, that runs when the parent component changes a value, and then updates the state of our component. This is a codesmell in general, and the issue it's pointing to is that we have a mismatch in state management. This component is trying to treat the same piece of state, visibility, as if it was both local state and external state. This is going to cause headaches in the long run, since the developer must manually keep local state in sync with external state whenever it changes. For example, since Glimmer Components are not 2-way data bound, there's no way for parents to know about child state changes, so for instance if this was how we activated the overlay in the parent: if (!this.isVisible) {
this.isVisible = true;
// do some other setup
} Then we would be back in the same boat as the previous example, where the overlay would never show again after it was closed the first time. So, how do we fix this, in a way that is not error prone? The answer is to always mutate the state where it originates, in this case in the parent component. We can update the Overlay component to receive a {{! -- templates/components/overlay.js --}}
{{#if @visible}}
<EmberWormhole @destinationElement={{@destinationElement}}>
<div class="overlay-container">
<div class="overlay" ...attributes>
{{yield (hash
close=@close
visible=@visible
)}}
</div>
</div>
</EmberWormhole>
{{/if}} The end result is actually a Template-Only component, since we're just passing arguments through. There are definitely times when this type of refactor will be a bit painful, and it can be tricky to figure out if something is local state, or if it belongs to the parent. I'm hoping we can get more examples so it'll be easier for users to figure out how to update their components, we're planning on building up a library of them in the Ember Atlas (I'll be adopting this one shortly!) |
While writing a brand new app in all-Octane has been very pleasant so far, this specific issue mentioned here has continued to be rather weird to me. I very much second the sentiment that As an example, we have a component that loads some data from an API. We use a task to load the data, and there are some arguments passed into the components that are used for the data loading, so if they change we want to trigger a reload. It would be hard to rewrite this with a getter, especially if you're at the same time trying to avoid juggling with promises everywhere. Here is a simplified example: export default class MyComponent extends Component {
@tracked items = [];
constructor() {
super(...arguments);
this.loadData.perform();
}
@restartableTask
*loadData() {
// Imagine this depends on some args...
let items = yield fetch(...);
this.items = items;
}
} {{#if this.loadData.isRunning}}
Loading...
{{else if this.items.length}}
Show items...
{{else}}
No items
{{/if}} Without adding an unnecessary wrapper div, this is not really possible at all. With helpers, it is possible, but honestly, it feels weird to put this into the template at all, as this data loading is not really a template concern. I do appreciate the manual specifying of the values that should be watched, though. Still, a helper makes 100% more sense to me for |
@mydea FWIW, the idea when we designed modifiers and removed lifecycle hooks was that in the long run, we should ideally develop helpers that allow us to consume data like tasks without manually restarting them. So for example, ideally your sample code would update the promise automatically if args changed, no need to call Some exploration in this space has been done with libraries like async-await-helpers, and I think that a consumption type API would make more sense for most of these use cases where args are triggering promises. Essentially, the goal is to take some imperative, side-effecting type code (like making an async request for data), and wrap it for usage in a declarative way, where the state is derived directly from incoming arguments and not necessarily manually called or started.
|
Does this addon/experiment by @pzuraq solve this issue? If so, maybe we can close this issue? |
Yes, definitely! ✅ |
RFC #415 Render Element Modifiers introduced the following render modifiers which, are available via the official
@ember/render-modifiers
package:{{did-insert fn ...args}}
: Activated only when the element is inserted in the DOM.{{did-update fn ...args}}
: Activated only on updates to its arguments (both positional and named). It does not run during or after initial render, or before element destruction.{{will-destroy fn ...args}}
: Activated immediately before the element is removed from the DOMThese helpers allowed to reduce the API surface area of the new
@glimmer/component
. The commonly used classic EmberComponent
hooks (didInsertElement
,didReceiveAttrs
,willDestroyElement
) are replaced with the respective render modifiers.In my opinion, this works extremely well for
{{did-insert}}
(didInsertElement
) and{{will-destroy}}
(willDestroyElement()
), which are used to setup and teardown element related state.There also are very valid applications for
{{did-update}}
, like this example from the RFC:Another big advantage of
{{did-update}}
over a genericdidReceiveAttrs()
/didUpdate()
hook is that you explicitly list the arguments you want to observe, whereas the generic hook would be re-evaluated whenever any argument changes. With{{did-update}}
you can also observe any other property on the component, whereas the hook only gets called when arguments to the component change.However, besides all the things
{{did-update}}
has going for it, I believe that it will often only be used as a workaround for the missingdidReceiveAttrs()
/didUpdate()
hook and that users will not actually use theelement
that is passed to thefn
then. For these scenarios,{{did-update}}
as an element modifier is just a hack. It also forces you to render elements to the DOM, which is not an option for "renderless" components that only{{yield}}
state.To better support these scenarios, I think we should provide complimentary template helpers, that behave exactly the same way, except for that they don't pass an
element
tofn
.For
{{did-insert}}
and{{did-update}}
this should be easily achieved with the public classic EmberHelper
API. For{{will-destroy}}
I don't think that it'll be possible with the public API.{{did-insert}}
{{did-update}}
{{will-destroy}}
Assuming that
willDestroy
is called for instances ofHelper
, which I am not sure about.I personally don't care much for
{{did-insert}}
and{{will-destroy}}
, but I think{{did-update}}
is crucial.For instance, I cannot update my
ember-link
addon to the Octane programming model, if I want to keep asserting the arguments that were provided to the<Link>
component. It was based onsparkles-component
, which still had andidUpdate
hook, which I used like:I can't use the
{{did-update}}
element modifier, since the template contains no DOM tags and only{{yield}}
s some state.The text was updated successfully, but these errors were encountered: