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

Update for insertBefore API change as per RFC287 #7

Merged
merged 2 commits into from
May 15, 2019

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented May 13, 2019

  • enforces insertBefore=null as only allowed value
  • Adds implementation for default replace semantics

This is a breaking change!

Closes #5

To Do:

  • Rebase after Ember Update #6 is merged
  • Decide if supporting pre-Glimmer versions using ember-wormhole is still worth the effort
  • Prepare to remove the -in-element-replace component from the build for Ember versions with public in-element support
  • Refactor to use the helper approach

@rwjblue
Copy link
Member

rwjblue commented May 14, 2019

@simonihmig - Have you considered using a helper instead of a component? I think it has the right semantics...

I made a demo Code Sandbox with some basic tests here.

import { helper } from "@ember/component/helper";

export default helper(function clearElement([element] /*, hash*/) {
  while (element.firstChild) {
    element.removeChild(element.firstChild);
  }

  return element;
});

@simonihmig
Copy link
Contributor Author

Have you considered using a helper instead of a component? I think it has the right semantics...

Yes, indeed I thought a while about it, but chose to use a component as I was not so sure if a helper would be the right abstraction here. Specifically, as it has no rendering related lifecycle hooks, I was not sure if it was safe to assume when it will run related to -in-element, if there could be some (future) Ember optimizations (caching, timing, lazy evaluation, what the heck) that could render those assumptions void etc.

I made a demo Code Sandbox with some basic tests here.

Ah, that looks really nice! What I specifically like here, being related to what I said above, is that by calling the helper as the target argument for -in-element and returning the element from the helper, we actually do have this guarantee effectively that the helper will have run before -in-element can do anything!

Will refactor my implementation later, thanks a bunch! 🙏

@rwjblue
Copy link
Member

rwjblue commented May 14, 2019

No problem! Was fun to hack on 😃.

@rwjblue
Copy link
Member

rwjblue commented May 14, 2019

Specifically, as it has no rendering related lifecycle hooks, I was not sure if it was safe to assume when it will run related to -in-element, if there could be some (future) Ember optimizations (caching, timing, lazy evaluation, what the heck) that could render those assumptions void etc.

Right, the thing about my implementation in the demo is that the private -in-element helper is passed the element by way of the helper. This ensures that by the time -in-element operates on it, it will have been cleared. It also guarantees that if the bound element value updates, the helper will be re-triggered and clear contents again (essentially with the same timing as if you had used {{-in-element this.someElement}} directly). Since -in-element and in-element are both permanently stable until the bound first argument changes, we have exactly the same stability guarantees with the helper solution that we do with in-element directly.

* enforces `insertBefore=null` as only allowed value
* Adds implementation for default replace semantics

*This is a breaking change!*
@simonihmig
Copy link
Contributor Author

@rwjblue updated the PR with your helper approach. Ready for another review, if you have the time!

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.

Looks great!

@simonihmig simonihmig changed the title WIP: Update for insertBefore API change as per RFC287 Update for insertBefore API change as per RFC287 May 15, 2019
@simonihmig simonihmig merged commit 058b6ed into master May 15, 2019
@simonihmig simonihmig deleted the enforce-insert-before branch May 15, 2019 06:51
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 this pull request may close these issues.

Does not account for replace by default semantics (or insertBefore=null)
2 participants