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

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

Closed
rwjblue opened this issue May 8, 2019 · 8 comments · Fixed by #7
Closed

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

rwjblue opened this issue May 8, 2019 · 8 comments · Fixed by #7

Comments

@rwjblue
Copy link
Member

rwjblue commented May 8, 2019

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
Copy link
Member Author

rwjblue commented May 8, 2019

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
Copy link
Contributor

@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: glimmerjs/glimmer-vm#918)

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

@cibernox
Copy link

cibernox commented May 9, 2019

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
Copy link
Member Author

rwjblue commented May 9, 2019

The RFC is correct for Ember usage, but the glimmer-vm was changed a bit more in glimmerjs/glimmer-vm#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
Copy link
Contributor

Here's a first stab at it: #7.
The relevant commit is this one: 3e5cb7c

@rwjblue would love your 👀!

@simonihmig
Copy link
Contributor

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
Copy link

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
Copy link
Member Author

rwjblue commented May 13, 2019

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants