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

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

Open
simonihmig opened this issue Sep 11, 2016 · 17 comments
Open

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

simonihmig opened this issue Sep 11, 2016 · 17 comments

Comments

@simonihmig
Copy link
Contributor

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

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

So put a div around that if

@simonihmig
Copy link
Contributor Author

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

rwjblue commented Sep 13, 2016

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

@rwjblue There you go...

@rwjblue
Copy link
Contributor

rwjblue commented Sep 25, 2016

FYI - We just submitted glimmerjs/glimmer-vm#331 to specifically support ember-wormhole for Ember 2.9...

bantic added a commit to bustle/ember-mobiledoc-dom-renderer that referenced this issue Oct 13, 2016
 * Fix failing tests when using ember with glimmer2.
 * Access DOM helper using a method that is safe for both pre-glimmer2 and
glimmer2.
 * Wrap the renderedMobiledoc documentFragment in a wrapper div
   (necessary for glimmer2 teardown, see
   glimmerjs/glimmer-vm#331 and
   yapplabs/ember-wormhole#66 (comment))
 * Fixes #18.
 * Update travis node version.
 * Update package keywords and description.
@urbany
Copy link

urbany commented Nov 29, 2016

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

rwjblue commented Nov 29, 2016

@urbany - With latest ember-wormhole and [email protected], 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
Copy link

urbany commented Nov 29, 2016

Thanks @rwjblue this solution also works for ember-tether

@shaunc
Copy link

shaunc commented Feb 20, 2017

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

bstro commented Mar 27, 2018

Is there a reason this addon does not use glimmerjs/glimmer-vm#331?

@lukemelia
Copy link
Contributor

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

lolmaus commented Mar 27, 2018

@bstro The reason is the API is not available yet? See the RFC: emberjs/rfcs#287

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

@krisselden
Copy link
Contributor

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

Is there any update on resolving this issue?

@lukemelia
Copy link
Contributor

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

@romgere
Copy link

romgere commented Mar 11, 2020

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

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

No branches or pull requests

10 participants