yapplabs / ember-wormhole

Render a child view somewhere else in the DOM.
MIT License
284 stars 68 forks source link

[Glimmer2] DOMException with {{#if}} helper #66

Open simonihmig opened 8 years ago

simonihmig commented 8 years ago

When testing for compatibility of ember-bootstrap with Glimmer2, I get the following exception with the modal component that uses ember-wormhole:

DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

I narrowed it down to ember-wormhole using a conditional statement, as shown in the following minimalistic example. As soon as show is set to true, I get the mentioned exception:

{{#ember-wormhole to="worm"}}
    <p>This is wormholed content.</p>

  {{#if show}}
    <p>This is conditional content.</p>
  {{/if}}
{{/ember-wormhole}}

Here is a demo repo to replicate this bug: https://github.com/simonihmig/ember-wormhole-glimmer2-demo

I am not sure who is to blame here, if it is ember-wormhole or Glimmer2, so first filing this here for the time being...

krisselden commented 8 years ago

Glimmer 2 caches the parent element, we need an API to tell glimmer it is reparented, for now you need to wrap dynamic content in a static element

krisselden commented 8 years ago

So put a div around that if

simonihmig commented 8 years ago

Thanks @krisselden for the quick feedback. Indeed, your suggestion fixes the problem!

If I understand you correctly this workaround will be obsolete once some changes to Glimmer have landed, is that right? Is that already tracked somewhere?

rwjblue commented 8 years ago

@simonihmig - Yes, it will likely be fixed by some API collaboration between ember-wormhole and Ember/Glimmer. To help facilitate that, it would be good to get a failing test PR submitted (notice that the canary build is currently passing).

simonihmig commented 8 years ago

@rwjblue There you go...

rwjblue commented 7 years ago

FYI - We just submitted https://github.com/tildeio/glimmer/pull/331 to specifically support ember-wormhole for Ember 2.9...

urbany commented 7 years ago

This is happening in the current ember release build 2.10.0 (and the tests are failing with ember-wormhole@master), anyway I can help?

rwjblue commented 7 years ago

@urbany - With latest ember-wormhole and ember@2.10, you need to have a stable root element inside the wormhole block. This is something that we will continue to iterate and work on, but for now the work around is fairly straightforward.

Change:

{{#ember-wormhole to="worm"}}
  {{#if foo}}

  {{/if}}
  <p>Other content, whatever</p>
{{/ember-wormhole}}

To:

{{#ember-wormhole to="worm"}}
  <div>
    {{#if foo}}

    {{/if}}
    <p>Other content, whatever</p>
  </div>
{{/ember-wormhole}}
urbany commented 7 years ago

Thanks @rwjblue this solution also works for ember-tether

shaunc commented 7 years ago

Is there a ticket in glimmer for the APIs to move content around and/or tell glimmer to ignore changes to a DOM subtree (or however this is going to work)? I have addons that also teleport content around, and need to figure out how to be compatible with glimmer. Wrapping in a static element doesn't seem to be enough for me...

bstro commented 6 years ago

Is there a reason this addon does not use https://github.com/glimmerjs/glimmer-vm/pull/331?

lukemelia commented 6 years ago

@bstro I think this addon will be rendered unnecessary once that API becomes public. I'm open to PR that reimplements on top of in-element without breaking API.

lolmaus commented 6 years ago

@bstro The reason is the API is not available yet? See the RFC: https://github.com/emberjs/rfcs/pull/287

But then there is this polyfill: https://github.com/kaliber5/ember-in-element-polyfill

krisselden commented 6 years ago

The {{in-element}} only is for append mode, if you change the target, in element clears the dom and runs the append program in the vm. In ember-wormhole just moves the dom on update, it does not rebuild it.

@runspired had a PR to try to get {{in-element}} to work with the update vm, but this is complicated because the same caching of parentNode in bounds that caused edge cases in wormhole, makes this more difficult.

For now, you can work around edge cases by ensuring the content in your wormhole block that is dynamic has a static element around it so its bounds parentNode will be stable (which is what all the tests have and why it works for @lukemelia ).

elwayman02 commented 5 years ago

Is there any update on resolving this issue?

lukemelia commented 5 years ago

@elwayman02 Not on my end, I'm sorry to say.

romgere commented 4 years ago

Just for information same error happens with {{component}} helper when not wrapped in a parent div.