detect props that aren't writable, fixes emberjs/ember.js#11221

[jayphelps]

Cc/ @stefanpenner

Working on tests, but want your input on the implementation while I do...

[stefanpenner]

this may be better then: https://github.com/tildeio/htmlbars/pull/352 but I wonder what examples beyond form require this functionality. I suspect in the world of custom WebComponents this may be the case, but i don't fully understand that scenario.

Can you share some further examples to help me understand this?

[jayphelps]

@stefanpenner

Absolutely. While I wouldn't be surprised if there were more prop/attr pairs in the spec with this behavior, my major concern is indeed custom elements.

class XPerson extends HTMLElement {
  @readonly
  age = 33;
}

document.registerElement('x-person', XPerson);

<x-person age={{someAge}}></x-person>

When I write his out, it actually indeed makes me wonder if properties truly marked as readonly via writeable: false should throw an error (or let the browser naturally throw it by writing to it) while properties which simply don't have a setter would instead just write to the attribute.

I can see cases both ways...So I'm unsure.

[jayphelps]

Summary: Do we need a distinction between "effectively readonly" and writeable: false?

AFAIK we can always setAttribute, but is there cases we this would be wrong and instead an error should be thrown? Because if someone has marked their prop as writeable: false, it may be very unintuitive that we set the attribute still.

[stefanpenner]

the input[type] scenario doesn't appear to be handled by this.

questions is, should it? Or should it be an additional whitelist

[jayphelps]

@stefanpenner If I'm following that thread correctly, IMO that's an unrelated case of "prop doesn't always reflect to attribute". The code for that might be in the same place, but unrelated concepts.

[jayphelps]

Okay, I think I understand better now, the issue is actually the fact that is errors, not that it doesn't reflect. (still unrelated, but yeah, fix is in the same code path. I'm attempting a PR right now)

[stefanpenner]

Okay, I think I understand better now, the issue is actually the fact that is errors, not that it doesn't reflect. (still unrelated, but yeah, fix is in the same code path. I'm attempting a PR right now)

some funkyness for sure. :+1:

[rwjblue]

Can haz rebase?

[jayphelps]

@rwjblue @stefanpenner @mixonic Given the discovery here https://github.com/tildeio/htmlbars/pull/353#discussion_r32170415, I've updated the solution to exclusively use a blacklist approach for all native elements. For Custom Elements, the robust solution is used for "ideal" interop. Whether or not setting the attribute when the property is not settable for Custom Elements is the right thing to do (vs. erroring or just not doing anything) is a bigger question of how third party web component libs like polymer, graffiti, etc expect things.

Please review and provide thoughts.

[jayphelps]

To further complicate things...in some ways we're going down the same path that React does. Check this out: https://github.com/facebook/react/blob/4c3e9650ba6c9ea90956a08542d9fa9b5d72ee88/src/renderers/dom/shared/HTMLDOMPropertyConfig.js

[stefanpenner]

Find me at the meetup tonight. I believe we have a much better long term fix. Although this may still be a nice interim solution.

[Robdel12]

Any updates on this? :)

[jayphelps]

@Robdel12 My latest understanding is that the fundamental way we choose whether we use the el.property = value vs setAttribute is likely to change. That said, @stefanpenner and I believed the issue would still exist for people choosing the wrong syntax (which doesn't exist yet) and we'd try and still fix it with this PR, but throw a warning of some kind.

I'm not the authority on this, just my understanding of it.

The idea is that <x-foo attr="{{bar}"> would use setAttribute because you put the mustache in quotes (HTML attributes are, after-all always strings) but if you omit the quotes <x-foo prop={{bar}> it would then use the same "props first if exists, setAttribute if not" pattern currently in place.

I'm personally in the camp of "this is going to confuse the hell out of people" because: previously we didn't really make it clear to devs that we were doing a props first, setAttribute second to begin with, most devs I've queried understandably don't realize there's a different between properties and attributes (I'm not joking), and maybe it won't be clear when to quote your 'staches and when not to. I'm super attached to this subject though, so I wouldn't be surprised if I was wrong about one or all of my concerns. I think it needs community input, likely an RFC. It's a tough problem because in almost every case we can choose props first and win, the dev doesn't need to know or care about the internals (which is why it was done) but these edge cases are brutal.

tl;dr

I hope this PR gets merged so people can move on, at least for now.

[Robdel12]

Thanks for the update! That helped me understand what's going on here.

[jamesarosen]

I don't know if it helps, but here's a JSBin that shows that

1. setting form via prop doesn't take
2. setting form via attr will cause <select>.form to be the new <form>
3. setting form via attr will cause the form to include the <select> when it submits

[jayphelps]

Superseded by #374