ember-polyfills / ember-in-element-polyfill

Polyfill for `in-element` as per RFC 287
MIT License
17 stars 6 forks source link

Does not account for replace by default semantics (or `insertBefore=null`) #5

Closed rwjblue closed 5 years ago

rwjblue commented 5 years ago

This polyfill is not conforming to the final public API from the RFC, specifically this section:

There is however one part of the behavior that the core team wants to make explicit before promoting the private API to public, and that is how the content is added to the destination when there is other content already there.

The desired behavior is that, by default, the rendered content will replace all the content of the destination, effectively becoming the its innerHTML. In the current behaviour the rendered content is appended as the end of any existing content. This will still be supported by passing insertBefore=null, but it will not be the default anymore. Any other value passed to insertBefore must produce an error.

rwjblue commented 5 years ago

While its easy to polyfill the {{in-element someEl insertBefore=null}} case (thats effectively what the private -in-element does by default), it will take a bit more work to support the new "replace by default" semantics.

An easy first step is to have the AST transform enforce always having insertBefore=null (and stripping it), that still doesn't fix the "replace by default" semantics but it would make the authored templates correct.

simonihmig commented 5 years ago

@rwjblue yeah, I am aware this needs some updates when the final public API arrives, but wanted to wait till things are more settled. I remember there were some discussions on Discord with @cibernox regarding the final semantics of insertBefore and specifically changing them a bit compared to the RFC.

Can you bring me up to date, is the RFC you linked to still the "source of truth", or were there some changes during the implementation? (Seems this was the Glimmer PR: https://github.com/glimmerjs/glimmer-vm/pull/918)

And is there already an ETA (version) for when this will land in Ember?

cibernox commented 5 years ago

I remember having some objections to the "insertBefore" part because it feels a little like reverse thinking to me, but there was technical reasons for that in the glimmer VM.

rwjblue commented 5 years ago

The RFC is correct for Ember usage, but the glimmer-vm was changed a bit more in https://github.com/glimmerjs/glimmer-vm/pull/931 (because Glimmer.js needed non-null insertBefore values).

And is there already an ETA (version) for when this will land in Ember?

Not specifically, but I think we'd hope for Ember 3.12 or 3.13. Exact timing is hard to tell though.

simonihmig commented 5 years ago

Here's a first stab at it: #7. The relevant commit is this one: https://github.com/kaliber5/ember-in-element-polyfill/pull/7/commits/3e5cb7c82f0782f5faec57a158d8e325ea07386d

@rwjblue would love your 👀!

simonihmig commented 5 years ago

Open question: this polyfill supports even pre-Glimmer versions through ember-wormhole. Not sure if it's really worth the effort to continue with that nowadays, given the additional steps required to implement the changes here? I would tend to drop it...

cibernox commented 5 years ago

given that in-element has existed for around 18 ember minor versions, I don't think it's necessary to support older versions than that.

rwjblue commented 5 years ago

I mentioned in #6, I'd personally drop support for < 2.18 (though I suspect it doesn't cost us much to keep support for 2.10+, so 2.12 is probably fine too).