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

HTML Attribute and Property Rationalization #314

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chadhietala
Copy link
Contributor

@Panman82
Copy link

Panman82 commented Mar 22, 2018

Sooo.... we're bringing back a variant of {{bind-attr}}?? 😕 (In regards to DX, no fun).

@knownasilya
Copy link
Contributor

Seems like an implementation of core element modifiers #112

The old way still exist, but works in a different way under the hood. The above would only be in certain situations. @chadhietala maybe an example is required of when the above would be required and when not?

@chadhietala
Copy link
Contributor Author

chadhietala commented Mar 23, 2018

@knownasilya Yes this is introducing more modifiers like #112 outlined. We are actively working on revising that RFC. This RFC may make more sense when #112 is updated.

@Panman8201 Not quite. {{bind-attr}} was used for introducing dynamic attributes to any element and still had the implicit semantics as described in this RFC. There are only a handful of places where we think {{prop}} would need to be used. Below is an outline of some unexpected consequences of how we do things today, along with some use cases we feel like will most likely would be affected.

Weird Cases That Work Today

These cases would require the {{prop}} modifier. For simplicity sake the below examples use static strings, but the result and migration is the same if they are dynamic.

innerText

<div class="red" innerText="Inner Text">
- What in the World???
</div>

Renders:

<div class="red">
Inner Text - What in the World???
</div>

Would be re-written to:

<div class="red" {{prop innerText="Inner Text"}}>
- What in the World???
</div>

innerHTML

<div class="red" innerHTML="<h1>Inner HTML</h1>">
- What in the World???
</div>

Renders:

<div class="red">
<h1>Inner HTML</h1> - What in the World???
</div>

Would be re-written to:

<div class="red" {{prop innerHTML="Inner HTML"}}>
- What in the World???
</div>

textContent

<div class="red" textContent="Text Content">
- What in the World???
</div>

Renders:

<div class="red">
Text Content - What in the World???
</div>

Would be re-written to:

<div class="red" {{prop textContent="Text Content"}}>
- What in the World???
</div>

More Reasonable Cases

As mentioned in the RFC majority of the time you want what happens by default. These cases should only pertain to built in "user input" elements and custom elements. Consider the following html:

<input value="Chad" />

When the browser parses this and creates the DOM it will setAttribute of value to "Chad", however if the user types into the <input /> the HTML attribute is not updated but rather the property on the HTMLElement. Example. So in Ember's case when you have a dynamic value that is being used on a "user input" element you would need to use {{prop}}. This would include properties such as value on <input />, checked on <radio />, selected on <option />. Happy to give more examples.

@balinterdi
Copy link

@chadhietala Would disabled and checked be other examples where you'll have to use {{prop}}?

- _SubExpression_

### `{{on}}` Runtime Semantics
The `{{on}}` modifier is installed on the element once the element is placed into the DOM. At that time we will call `addEventListener` with the `EventName` and `EventCallBack`. Since an element can have multiple `{{on}}` modifiers installed on it, we will associate the `EventCallBack` and `EventName` with the element so we can call `removeEventListener` during destruction of the element. The `addEventListener` will be called with `{ capture: true }` if `EventOptions` are not passed.

Choose a reason for hiding this comment

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

@chadhietala Why capture phase as default instead of bubbling phase? Any reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be outdated. There was a separate RFC for {{on}} modifier, which is merged. It has been implemented and shipped as part of Octane.

@piotrpalek
Copy link

I dislike this for DX reasons mostly. It's pushing a lot of manual labor to developers and additional complexity to the templates. I'd argue that the templates will get unreadable with all of the sigils in them, it's already somewhat hard to read some templates when there is a lot of stuff going on.

Also, do we really need yet another way to define actions / events? I think our current setup is complex enough:

image

I guess my main issue is that the current setup mostly works and is pretty convenient to work with. It feels like this proposal optimizes for the edge cases instead of the "90%" use case. Maybe an alternative would be to go the other way, meaning leave the heuristics as they are (or improve if necessary) and provide tools (like {{prop}} or {{on}}) to specify what we want to do in the instances we need to.

@chadhietala
Copy link
Contributor Author

@balinterdi Yes.

@piotrpalek For what it's worth I help maintain an app that has 120,000 lines of handlebars (2589 templates). I did a quick audit of just <input /> and <option /> usage, of the 752 usages (268 <option /> and 484 <input />) identified roughly 54 places where {{prop}} would need to be used.

@piotrpalek
Copy link

piotrpalek commented Mar 25, 2018

@chadhietala I've tried to collect my thoughts about this proposal:

What I don't like:

  • it will force existing and new Ember.js users to learn the (muddy) attributes vs properties html semantics
  • it makes using Ember.js less convenient
  • it adds complexity to things we already have (event listeners/action handlers)
  • it increases (imo significantly) the learning curve of Ember

What I wish it would looks like:

  • I would expect of Ember to do the right thing for the common case by default (as it does now)
  • Maybe these element modifiers could be added, but we wouldn't deprecate/remove/discourage the current behavior? This would allow their usage for those special cases without forcing new/existing users to learn when to use which
  • If the current semantics are unclear perhaps I'd be good to specify them somewhere to clear it up (could also have the benefit of exposing uncosistencies / bugs)
  • If the Custom Components use case is the most important one, maybe we could use the same way Glimmer.js distinguishes between properties and attributes (@ sigil vs lack of it)

Useful discussions from the React ecosystem:

@cibernox
Copy link
Contributor

While I'm not opposed to this RFC per-se, I think we're approaching it the wrong way.

I'd much rather see #112 advance and hold this back. Once we have element modifiers we should experiment with the proposed API in this RFC (which sounds reasonable to me) in addon-land and eventually promote it into core.

But in any case I'm agains adding more {{bind-attr}}-like magic until that power is generally available to users.

Copy link
Contributor

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

I have struggled recently with custom elements and ember a lot. This RFC would simplify that a lot.

I'm wondering that it doesn't mention backward compatibility. Setting values as attribute which have been set as property before is a breaking change, isn't it?

There seems to be a need to:

  1. Allow developers to opt-in to attributes only semantics.
  2. Deprecate the current behavior.

Maybe an optional feature?

- _NumberLiteral_

### `{{prop}}` Runtime Semantics
The `{{prop}}` modifier is installed on the element once the element is placed in the DOM. At that time it will loop over the `HashPair`(s) setting the properties on to the element. No normalization is done for the `Key` or `Value`. In other words what you see is what you get.
Copy link
Contributor

Choose a reason for hiding this comment

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

The timing is problematic for custom elements.

Custom elements (web components) have lifecycle hooks similar to @ember/component. One of them is connectedCallback. It's executed when the element is insert into the DOM.

The property is not available in that hook if {{prop}} modifier runs after the element had been insert into DOM.

I'm aware that this is a limitation of modifiers in general and not related to {{prop}} modifier proposed here only. The limitation is discussed in detail at #652.

- _SubExpression_

### `{{on}}` Runtime Semantics
The `{{on}}` modifier is installed on the element once the element is placed into the DOM. At that time we will call `addEventListener` with the `EventName` and `EventCallBack`. Since an element can have multiple `{{on}}` modifiers installed on it, we will associate the `EventCallBack` and `EventName` with the element so we can call `removeEventListener` during destruction of the element. The `addEventListener` will be called with `{ capture: true }` if `EventOptions` are not passed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be outdated. There was a separate RFC for {{on}} modifier, which is merged. It has been implemented and shipped as part of Octane.

The drawback of this is that we are making developers know about some nuanced details of the updating semantics of HTMLElements. The tradeoff is that serve-side rendering works as expected and developers have a clear path for adopting natvie web components.

## Alternatives
There are a couple different alternatives. We could keep `{{textarea}}` and `{{input}}` around and somehow special case them as special components that always set properties. However, this means we would have to expand helper support to things like `<option />`. It's also odd to be creating primitives just for the sake of updating semantics.
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have missed it but is this RFC proposing to deprecate <Textures> and <Input> built-in components?


- Can we remove `TextSupportMixin`?
- Can we remove `{{textarea}}`?
- Can we remove `{{input}}`?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how these questions are related to this specific RFC.

- Can we remove `{{textarea}}`?
- Can we remove `{{input}}`?
- Do we really want `{{on}}` to have updating semantics for `EventName`?
- Who is responsible for binding the context for the `EventCallback`?
Copy link
Contributor

Choose a reason for hiding this comment

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

These two questions seem to be outdated as on modifier has already been shipped.

## Drawbacks
The drawback of this is that we are making developers know about some nuanced details of the updating semantics of HTMLElements. The tradeoff is that serve-side rendering works as expected and developers have a clear path for adopting natvie web components.

## Alternatives
Copy link
Contributor

Choose a reason for hiding this comment

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

As a third option we could stick with the current rules but additionally provide fine granular control to developer where needed. E.g
{{attr}} and {{prop}} modifiers that run before the element is insert into the DOM.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

Is there still interest in moving this forward? If so, I can help get it finalized.

@wagenet wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Proposed In the Proposed Stage T-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants