Skip to content
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

Modifiers #353

Closed
wants to merge 12 commits into from
Closed

Modifiers #353

wants to merge 12 commits into from

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Aug 16, 2018

@rtablada
Copy link
Contributor

@chadhietala what does this mean for overloaded helpers vs modifiers?

Like action is both?

Is that something we want to allow as public API for other users?

How do we keep that compatible with current helpers and AST transforms like https://github.com/mmun/ember-component-attributes?

@chadhietala
Copy link
Contributor Author

@rtablada {{action}} is implemented twice in Ember, both as a modifier and helper. You would be able to do that same thing in your app. The position here is what is important, a MustacheStatement in element position is only ever going to lookup a modifier.

@mike-north
Copy link
Contributor

Do you have any concerns around making it easy to static-analyze potential misuse of helpers and modifiers? As it stands, we'd have to know more about the implementation of these two things (my-helper, my-modifier) to detect problems at build-time.

<button onClick={{my-modifier "does not emit a value"}} >
<button {{my-helper "emit a value into nothingness"}}>

This hook has the following timing semantics:

**Always**
- called **after** any children modifiers `didInsertElement` hook are called
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'any' => 'all'?

This hook has the following timing semantics:

**Always**
- called **after** any children modifier's `willDestroyElement` hook is called
Copy link
Contributor

@xg-wang xg-wang Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'any' => 'all'? or before any children. willDestroyElement sounds more like a hook called before the recursive destruction.

@chadhietala
Copy link
Contributor Author

chadhietala commented Aug 21, 2018

@mike-north this is going to result in a compilation error. In the case of the modifier you would get an error like

No modifier named “my-helper” could be found

In the case of the modifier being used outside of element space you will get a helper not found error.

Effectively the location narrows the type.


**May or May Not**
- be called in the same tick as DOM insertion
- have the the parent's children fully initialized in DOM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall very positive to this RFC!

Minor thing: I guess "the parent's children" is the same as "the elements siblings"? If that's the case, would the term sibling be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like clarification. If children is sibling elements, can't those belong to sibling components?

MU paths:

src/ui/components/flummux/element-modifier.js
src/ui/routes/posts/-components/whipperwill/element-modifier.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there's a discrepancy between the type name of element-modifier and the base package + class names of @ember/modifier + Modifier. Is there a reason to have the leading element for one but not the other?

Relatedly, how does a dashed type name play with Rule 3 of the MU resolution rules? What would the identifier for a named element-modifier export be?

@luxzeitlos
Copy link

I'm not sure what to think about that we increate the API surface for the application developer to make the API for the addon developer simpler.

Not many people will ever write a component manager, but a lot of developers will use modifiers. And I think the story about {{action}} shows that its quite confusing to have an helper and a modifier with the same name. Maybe we could have a different syntax for this?

Oh, and can I pass a modifier to a glimmer component and later use it with ...attributes? :trollface:

@gossi
Copy link

gossi commented Aug 25, 2018

Some notes that I quickly come up with:

  1. Modifiers !== Components. period. So don't put them into src/ui/components but e.g. src/ui/modifiers. Don't mix. It just happens I have to work on components that were helpers (and placed there), it is messy. Lesson learned, please don't repeat.

  2. As the Component is basically the umbrella to encapsulate the logic around a component (or to bridge it to a 3rd party library), this is now spread across various modifiers?

  3. Will these modifiers be connected to the Component (if there is one) or will they work in isolation? If so, the concept of a component managing its template/DOM is kinda eliminated? Or that's how I feel about it at the moment.

  4. With helper, modifier and Component it increases complexity. There should be a clear path forward. E.g. <div {{my-whatever}} ...> has to be clear where to find it in the code and what it is.

Although I see the benefits of modifiers (e.g. attaching listeners to elements wo/ js code). At the same time I see this critical as well as we will now declaratively set our listeners which is done programmatically in vanilla JS. With native classes Ember is catching up with recent javascript developments modifiers it looks like there is a chance we are splitting apart again? Components mentally always connect to Web Components (and shadow DOM in some way). Ember should more align with these concepts in order to onboard new ember devs.

@chadhietala
Copy link
Contributor Author

chadhietala commented Aug 25, 2018

@luxferresum as mentioned before there are 2 implementations of {{action}} today that has the interfaces aligned, however they behave different. The {{action}} helper returns a bound method that is either assigned directly to an element or is closed over and passed. The {{action}} modifier gains access to the underlying element and registers data- attributes that are used by the event dispatcher.

To draw corollary, JS decorators have different implementations based on where the decorator is used. For instance a class decorator cannot be used as a accessor decorator unless the implementation allows for both.

@gossi some answers to your questions

  1. Module Unification has already specified that "components" is a generic term that encompasses components, specialized components, helpers, and modifiers.

  2. As mentioned in the RFC Glimmer components are effectively "fragments" and thus there is not a single this.element that represents the component, so we need a way to give raw assess to the underlying DOM. I wrote another RFC about providing components raw access to the underlying DOM and there were many issues with it.

  3. Does not foreclose on the idea see the alternatives section in this RFC.

  4. Position narrows the type.

@luxzeitlos
Copy link

I do understand how {{action}} works. I wanted to point out that currently {{action}} is quite hard to teach and there are many blog posts about how it works. So I hope we will eventually get rid of classic actions and don't reintroduce anything like this again with modifiers.

@luxzeitlos luxzeitlos mentioned this pull request Aug 25, 2018

Glimmer components have `outerHTML` semantics, meaning what you see in the template is what you get in the DOM, there is no `tagName` that wraps the template. While this drastically simplifies the API for creating new components it makes reliable access to the a component's DOM structure very difficult to do. As [pointed out](https://github.com/emberjs/rfcs/pull/351#issuecomment-412123046) in the [Bounds RFC](https://github.com/emberjs/rfcs/pull/351) the stability of the nodes in the `Bounds` object creates way too many footguns. So a formalized way for accessing DOM in the Glimmer component world is still needed.

Element modifiers allow for stable access of the DOM node they are installed on. This allows for programatic assess to DOM in Glimmer templates and also offers a more targeted construct for cases where classic components were being used. The introduction of this API will likely result in the proliferation of one or several popular addons for managing element event listeners, style and animation.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/programatic assess/programmatic access/?

@samselikoff
Copy link
Contributor

This is an awesome RFC, exactly the kind of low-level primitive Ember needs to foster more experimentation. I think the power of abstractions that can be made with these sorts of things are under appreciated.

<button {{on 'click' (action 'save')}}>Save</button> might be a more enticing example (on vs. add-event-listener).

I think these modifiers could be used for so many useful things – animation for one. They are more like a horizontal composition/mixin pattern than a hierarchical/tree-like composition pattern (and many problem areas lend themselves to horizontal rather than vertical methods of reuse). Cannot wait to get my hands on these!

@sandstrom
Copy link
Contributor

@chadhietala Great examples! 🏅 We've built something similar to the performance marking example, but with components. Using modifiers would be much cleaner!

And although I liked the preceding RFC, this one is easier to understand and more powerful/generic. 👍

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I am excited to see this move forward again! Thanks for picking it up @chadhietala!

General questions I have after reviewing:

  • This RFC doesn't specify how modifiers interact with angle bracket invocation. I think it should be specifically called out (either that we support them on angle invocations and what that means or that we don't).
  • Based on the naming, I had assumed that didUpdate would be called both when incoming arguments have changed and when the underlying element was updated (e.g. an attribute value was updated/changed/etc). Did you consider that case and decide that we shouldn't invoke didUpdate?
  • How could a modifier add attributes (e.g. classes, style, selected, etc) before inserted into the DOM? It seems like the earliest hook we have is didInsertElement, but the guarantees there are that it has already been inserted. This would result in FOUC in some cases (imagine a <div {{style this.styles}}></div> modifier) and broken behaviors in others (where specific attributes must be present when inserted into the DOM for them to function properly). Do we need a willInsertElement (or possibly even just pass the element in to the constructor)?


## Motivation

Classic component instances have a `this.element` property which provides you a single DOM node as defined by `tagName`. The children of this node will be the DOM representation of what you wrote in your template. Templates are typically referred to having `innerHTML` semantics in classic components since there is a single wrapping element that is the parent of the template. These semantics allow for components to encapsulate some 3rd party JavaScript library or do some fine grain DOM manipulation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe s/classic components/Ember.Component/ ?


Classic component instances have a `this.element` property which provides you a single DOM node as defined by `tagName`. The children of this node will be the DOM representation of what you wrote in your template. Templates are typically referred to having `innerHTML` semantics in classic components since there is a single wrapping element that is the parent of the template. These semantics allow for components to encapsulate some 3rd party JavaScript library or do some fine grain DOM manipulation.

Glimmer components have `outerHTML` semantics, meaning what you see in the template is what you get in the DOM, there is no `tagName` that wraps the template. While this drastically simplifies the API for creating new components it makes reliable access to the a component's DOM structure very difficult to do. As [pointed out](https://github.com/emberjs/rfcs/pull/351#issuecomment-412123046) in the [Bounds RFC](https://github.com/emberjs/rfcs/pull/351) the stability of the nodes in the `Bounds` object creates way too many footguns. So a formalized way for accessing DOM in the Glimmer component world is still needed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Glimmer components/custom components/ (we don't have "glimmer components" yet 😛, but "custom components" are included in 3.4.0).

<b {{crum bing='whoop'}} zip="bango">Hm...</b>
```

Element modifiers may be invoked with params or hash arguments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean they can accept positional and/or named arguments, right? As written this infers only positional or named are allowed at once (and that you can't use both).

src/ui/routes/posts/-components/whipperwill/modifier.js
```

In Module Unification, modifiers live within the generalized collection type "components" [as specified](https://github.com/dgeb/rfcs/blob/module-unification/text/0000-module-unification.md#components). Modifiers, like component and helpers, are eligible for local lookup. For example:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this link should probably not reference dgeb's fork (I think the same link works on emberjs org too?)...


#### `willDestroyElement` semantics

`willDestroyElement` is called during the destruction of a template. It receives no arguments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here seems odd. It doesn't seem that "destruction of a template" is actually the limit of willDestroyElement. For example:

{{#if someCondition}}
  <div {{flummux foo bar}}></div>
{{/if}}

When someCondition change from true to false, I would expect flummux's willDestroyElement to be called but this is unrelated to "destruction of the template".

This hook has the following timing semantics:

**Always**
- called **after** all children modifier's `willDestroyElement` hook is called

This comment was marked as resolved.

This comment was marked as resolved.


## Alternatives

The alternative to this is to create a "ref"-like API that is available in [other client side frameworks](https://reactjs.org/docs/refs-and-the-dom.html). This may look like the following:

This comment was marked as resolved.

@gossi
Copy link

gossi commented Aug 29, 2018

@chadhietala I did some rereading and get a better understanding. Some of this is now more clear to me.

  1. For how I understand this:
  • modifiers: will be placed inside an element tag, e.g. <div {{on 'click' (action 'huibuuh')}}>
  • helpers: will be placed outside of an element tag, e.g. {{log 'blah'}}

If that's correct, please add it somewhere at the top to give an immediate better understanding.

  1. Can you please provide two more examples, that show the interaction of modifiers with a controller/component. And a second one (more in direction towards MU) how a component will work with local modifiers and how the file layout will look like (more to give us an impression what we will be able to do with Ember Octane 🎉).

  2. Although the MU RFC mentions to put everything component related into src/ui/components I still think, this is a bad idea. It's good to group them under src/ui but should be src/ui/helpers, src/ui/modifiers, ... maybe there will be a future RFC. Just want to rise awareness for now.

Questions

  1. With handlebars you rarely interact with the DOM directly (unless applying third party libraries). Will modifiers be the (only?) location where you should do this?

  2. That would be a follow up on @rwjblue of passing the element in the constructor to apply and manage attributes. I do like this idea. Though contrary to this, this will be an unknown set of attributes managed by a modifier that are non visible in the template (hidden UI). E.g. just use attributes as we would of today: <input disabled={{this.disabled}}>. The question to this is: Is there a use-case where a modifier MUST be used instead of just going the regular way we already have.

@buschtoens
Copy link
Contributor

Motivated by #415 (comment) (@ember/render-modifiers) I've published ember-on-modifier, which implements the exemplary {{on}} modifier.

@luxzeitlos
Copy link

@buschtoens why would I do {{on "click" this.foo}} instead of onclick={{this.foo}}? Also whats is there a difference between <div {{on "click" this.foo}}> and <div {{action this.foo}}>?


And how can willInsertElement implemented while modifier managers when modifier managers dont have an even for this moment? Dont we need this first?

@knownasilya
Copy link
Contributor

knownasilya commented Feb 14, 2019

@luxferresum element modifier managers RFC was merged and the feature implemented here: emberjs/ember.js#17143

Regarding <div {{on "click" this.foo}}> vs <div {{action this.foo}}>, the first option is nicer because it's clear that it's for click and much easier to understand and read. Also action is overloaded.

Regarding {{on "click" this.foo}} instead of onclick={{this.foo}}, the first option can handle events in a more performant way in the future, plus you can pass in options to the event listener.

@luxzeitlos
Copy link

@knownasilya But installModifier, the first event that receives the element, will always be "called after DOM insertion".

@knownasilya
Copy link
Contributor

knownasilya commented Feb 14, 2019

@luxferresum maybe by caching the value and element?

Ah, I don't think willInsert will be a thing, because you can handle that in the constructor (init for classic components) which fires before didInsert. The work around is to fire did-insert on a parent element I think, due to DOM traversal.

At least for now. It looks like https://github.com/emberjs/ember-render-modifiers is the way forward.

@mixonic mixonic mentioned this pull request Mar 2, 2019
bors added a commit to rust-lang/crates.io that referenced this pull request Dec 21, 2019
Convert curly component to angle bracket component invocations

see https://emberjs.github.io/rfcs/0311-angle-bracket-invocation.html

the main advantage from this is that arguments and attributes are both first-class citizens now and components invoked with angle bracket syntax can use "modifiers" (see emberjs/rfcs#353)
@chriskrycho
Copy link
Contributor

Hey folks, what's the status on this?

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Jan 21, 2020

Based on the proposed implementation, and how everyone is using https://github.com/ember-modifier/ember-modifier,
I think this RFC needs updating.

Though, the email about your comment got me thinking -- did Modifiers make it in without an RFC?
https://github.com/emberjs/rfcs/blob/master/text/0373-Element-Modifier-Managers.md
nope! Modifier Managers RFC is what landed

<FooBar (draggable x=x y=y) />
```

This also means that they have no relationship to `...attributes`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this, that means that a developer would not be able to forward the modifier. If so:

  • What are the drawbacks of forwarding a modifier?
  • What does that means for the {{on}} modifier? (currently can be forwarded)
    • Not giving modifiers the ability to forward and not changing {{on}} creates an inconsistent API
    • Changing {{on}} would be a breaking change

Thoughts?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is simply outdated information, as modifiers are "forwarded" in ...attributes. The RFC for that was created and merged after this RFC was last updated: #435

@chriskrycho
Copy link
Contributor

chriskrycho commented Feb 21, 2020

Given that ember-modifiers has been implemented and has a nice API, is there anything blocking updating this text and merging it, with ember-modifiers as the official API? If so, what are the blockers? I.e. is there a path where we can ship a public modifier API for other addons to use, even if we end up needing to iterate on that design later?

@jelhan
Copy link
Contributor

jelhan commented Aug 10, 2020

Given that ember-modifiers has been implemented and has a nice API, is there anything blocking updating this text and merging it, with ember-modifiers as the official API? If so, what are the blockers? I.e. is there a path where we can ship a public modifier API for other addons to use, even if we end up needing to iterate on that design later?

The modifier manager is not supporting modifying an element before it's insert into DOM. This limits some use cases, which should be supported. Please find more details about this in #652.

The public API for functional modifiers exposed by ember-modifier doesn't seem to be flexible enough to support an additional lifecycle hook, which runs before the element is inserted. Not saying that such will be added. Not sure if it should be usable through a functional modifier approach if it lands. But we should resolve these questions before committing to a public high-level API for writing modifiers.

@wycats
Copy link
Member

wycats commented Aug 18, 2020

Given that ember-modifiers has been implemented and has a nice API, is there anything blocking updating this text and merging it, with ember-modifiers as the official API? If so, what are the blockers? I.e. is there a path where we can ship a public modifier API for other addons to use, even if we end up needing to iterate on that design later?

@chriskrycho from my perspective, the biggest issue is unifying a bunch of features (modifiers, helpers, destroyables and ultimately usables) around a single high-level API.

From the perspective of modifiers, the biggest inconsistency between what I hope we ultimately do and what ember-modifiers does is: https://github.com/ember-modifier/ember-modifier/blob/master/addon/-private/class/modifier-manager.ts#L37

In the absence of a modifier supplying updating hooks, I would like these APIs to default to teardown/recreate across the board, which would be a breaking change if we stabilized ember-modifier into Ember right now.

@lukemelia
Copy link
Member

Given that ember-modifiers has been implemented and has a nice API, is there anything blocking updating this text and merging it, with ember-modifiers as the official API? If so, what are the blockers? I.e. is there a path where we can ship a public modifier API for other addons to use, even if we end up needing to iterate on that design later?

Per this open issue, the question of whether and how args should be passed to destroyModifier should be reconsidered. As the issue describes, the current spec'd behavior as implemented by ember-modifier (destroy consuming the modifier's args) makes it easy to create confusing bugs.

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Aug 18, 2020

I think we discussed that using a proxy would allow lazy consumption of tracked tags (which solves the destroy issue). I volunteered to implement that behavior, but am waiting on some refactoring work to land from @pzuraq

@wycats
Copy link
Member

wycats commented Aug 18, 2020

@lukemelia I totally agree with this. In general, destructors should be used in a way that's compatible with JS' new finalizer feature, which goes further than your request here and actually doesn't even make the object itself available to finalizers. This is because it's way to easy to accidentally resuscitate an object in its own finalizer, resulting in confusing bugs.

Finalizers address the problem by allowing you to supply a different piece of metadata that gets passed into the destructor, so you can do parameter-specific things during destruction without waking the object back up.

That was a long-winded way of saying I agree with your design proposal, and also think we should consider aligning RFC #580 (destroyables) with the finalizer design. @pzuraq

Note that we would not want to use the finalizer design directly for our destroyables, because we want to offer more timing guarantees for resource cleanup. Our destroyables are closer to the explicit resource management JS proposal.

(I mentioned the finalizer design because @lukemelia made me realize that the kinds of bugs that the current design enables are similar in spirit to the kinds of mistakes that the finalizer design is trying to avoid).

@NullVoxPopuli
Copy link
Sponsor Contributor

Was this accidentally implemented? Do we still need this RFC

@jelhan
Copy link
Contributor

jelhan commented Jan 29, 2022

Was this accidentally implemented? Do we still need this RFC

I'm not sure if the RFC is this still needed. But I don't think it's implemented.

ember-modifiers is not part of default blueprint. User-defined modifiers are also not captured in the guides.

Maybe the RFC could be updated to add existing ember-modifiers package to application blueprint and cover it in the guides?

@NullVoxPopuli
Copy link
Sponsor Contributor

With #757
I don't think we need ember-modifier to be in the blueprint

@jelhan
Copy link
Contributor

jelhan commented Jan 29, 2022

With #757 I don't think we need ember-modifier to be in the blueprint

I'm not sure if I agree. That would make state-less, function-based modifiers a first-class concern with built-in support while state-full, class-based modifiers are user-land. If one is teached in the guides but the other not, that may make discoverability of the other option difficult.

@NullVoxPopuli
Copy link
Sponsor Contributor

It's an improvement over today's current state of the discoverability of any modifiers difficult.

but, I'd say most modifiers don't actually need state. State can be useful, but I think we'd want a separate RFC for figuring out class-based modifiers as default.

@SergeAstapov SergeAstapov mentioned this pull request Mar 29, 2022
@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

Is this something we still need? We do have modifiers now.

@locks
Copy link
Contributor

locks commented Jul 24, 2022

As mentioned in the description of the mentioned PR, #811 supersedes this, so we can close it!

@locks locks closed this Jul 24, 2022
@locks locks deleted the new-modifiers branch July 24, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-framework RFCs that impact the ember.js library T-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet