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

Render / replaceNode unexpected behavior? #2004

Open
richardhinkamp opened this issue Oct 12, 2019 · 13 comments
Open

Render / replaceNode unexpected behavior? #2004

richardhinkamp opened this issue Oct 12, 2019 · 13 comments

Comments

@richardhinkamp
Copy link

I'm having some issues with render(), followup to #2002
I have an app which is mostly not preact based, but uses a custom made system to generate HTML. We are migrating to preact, so different parts of the app are now created in preact. We currently use preact 8, and use this pattern:

const container = document.getElementById('someId');
let rendered = preact.render(<SomeComponent />, container);

// based on some events an update of the component may be required
preact.render(<SomeComponent />, container, rendered);

// base on another event it must be removed
preact.render("", container, component);
rendered = undefined;

This works like a charm.

Now I'm trying to migrate to Preact X and I'm facing some issues, because render changed. Maybe I do not understand the desired approach for this, but I found some strange effects using render():

https://codepen.io/rhinkamp/pen/abbdBgE
The rendered component is not appended to container, but added before the h1.

https://codepen.io/rhinkamp/pen/ExxPNOv
A existing div in the HTML is replaced by the component, but it's position changed to before the h1.

https://codepen.io/rhinkamp/pen/WNNroVQ
Using the replaceNode parameter it's in the correct place, but the nodes look merged, since it's the component and the id of the original node still present.

https://codepen.io/rhinkamp/pen/eYYJgGV
If the replaceNode is a different type (span opposed to div) it is not merged?

https://codepen.io/rhinkamp/pen/gOOPLVJ
A button to re-render the component with a different text suddenly drops the "Bar:" text on the first click. On the second click "Bar:" is back, but "allo" is missing.

https://codepen.io/rhinkamp/pen/XWWXpgE
Using an empty container node and no replaceNode option does work correct.

https://codepen.io/rhinkamp/pen/OJJMWJL
A button to remove the element and a button to add/update. This seems to work, but after removing and adding, clicking the add/update button again will remove all text from the component?

Are these cases correct but am I expecting the wrong thing, or are these bugs?

Is there a desired approach for adding preact component to an existing app? Especially adding it somewhere into the existing DOM tree.

I think I can create container elements to render components in, so I don't have to use replaceNode, I guess that will fix most if not all issues, but the replaceNode does result in some unexpected behavior?

@richardhinkamp
Copy link
Author

https://codepen.io/rhinkamp/pen/OJJMWzM
Rendering 2 components in 1 containerNode will result in 1 rendered component instead of 2, is that supposed to happen?

@JoviDeCroock
Copy link
Member

Hey @richardhinkamp

Thank you so much for all the reproductions

1: isn't working
2: Yes render without replacing leaves pre-existent nodes intact.
3: Is fixed on master, this was indeed a bug
4: isn't working
5: isn't working
6: isn't working
7: isn't working
8: isn't working

Most of these are dependency problems and I've got them a lot when opening codepens they don't seem to correctly save the deps :/ codesandbox is a bit more reliable in that aspect.

@richardhinkamp
Copy link
Author

I'm not able to change them to codesandbox now. The only external script is https://unpkg.com/preact/dist/preact.umd.js, if it's missing you can maybe add that and it should work I hope.

@developit
Copy link
Member

@richardhinkamp just a note - in your last demo, the target element reference is obtained once and held globally, but the button onclick actually removes that element from the page and replaces it with a Text node. That part is actually intended behavior, and the same as Preact 8. The reason this produces strange results is because target is passed again to the third render() call, but at that point it's referring to an element that is not present in the DOM tree.

@richardhinkamp
Copy link
Author

@developit yeah I get that, so I thought it would ignore the replaceNode since it's not a child of the container, but ala. Weird thing still is, after removing and adding, multiple clicks on add/update will result in empty div / div with Bar:allo alternately.

@richardhinkamp
Copy link
Author

@JoviDeCroock regarding case 2:

2: Yes render without replacing leaves pre-existent nodes intact.

Well the problem is that <div>Hi</div> is not left intact but replaced with the rendered component and move to another position, before the <h1> instead of after.

How can I test the latest master in a codepen or codesandbox?

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Oct 14, 2019

You can pull in the code --> build and copy paste in the built CJS file that's only way I think. This is a pretty extensive issue for me to validate so I'll probably need to find a day where I have a few hours to spare.

@bernardop
Copy link

Sorry to piggyback on this issue, but sounds similar to an issue I'm experiencing. When rendering a component, then removing it using replaceNode and then trying to render it again, it seems that the component is being rendered, but without any attributes. Here's a demo:

https://codesandbox.io/s/charming-euclid-540dp

Am I doing something wrong?

@yashrajpchavda
Copy link

Hey @JoviDeCroock about the issue #3 reported by richardhinkamp, I checked it in the latest release 10.1.1 and I still see some problem with it, have created a sandbox with reproduction.

https://codesandbox.io/s/sweet-beaver-1nbqe

When replaceNode is specified, it merges the nodes instead of replace how it used to do in preact8. Is the fix yet to be released to npm?

@developit
Copy link
Member

developit commented Dec 26, 2019

@yashrajpchavda the demo for #3 has a bug that is making it look like Preact is doing something wrong, but it's actually doing the right thing. The <Foo /> component being rendered with replaceNode=getElementById(target) doesn't include id="target" on its root node. That means that, when triggering a second render, there is no element with an id "target" present in the DOM anymore. Here's a version of the codepen example modified to include id=target on the root element in JSX that shows it to be working.

However, your codesandbox is actually demonstrating a different bug: when the root node of a tree doesn't match the DOM element you passed in via replaceNode, Preact X currently creates a new tree but does not destroy the mismatched old one. In your demo, the DOM element passed to replaceNode is the <a>, but the JSX rendered is a <span> containing an <a>. This is a bug.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Dec 26, 2019

@developit The issue in the above example is also the switch of parent I think since if we'd just reuse document.body this would work perfectly. This confuses me though

@yashrajpchavda
Copy link

yashrajpchavda commented Dec 27, 2019

Aah okay, yes @developit now when I look at the codepen for issue 3, it makes more sense that it is a different one than what I experienced. Basically in my scenario, I have an anchor which I need to wrap with another component to add some feature to the anchors with mailto href and replace them with original in the DOM.

@developit
Copy link
Member

Hi all,

Sorry for the radio silence on this one. I just came back to the issue and I've got a little guide here to help make sure everyone is on the same page. TLDR is: there is technically a bug here, but it exists in an undocumented feature we had in Preact 8.

Here's a retrofitted/commented version of @yashrajpchavda's great CodeSandbox demo with some additional explanations and an example of similar behavior we have today that does actually work in Preact X:
https://codesandbox.io/s/preact-differing-root-subtree-render-bug-jywti

I'll break it down here step-by-step though.

First, we'll render a DOM tree using Preact:

render((
  <div>
    before
    <a id="link" href="mailto:[email protected]">EMail Me</a>
    after
  </div>
), document.body)

Now, say we wanted to specifically update that <a> in-place, without having to re-render the whole tree again. In Preact 8 we diffed against the DOM, so it was possible to "pinpoint" an element in the DOM, and call render() directly on it with a new tree. This might have done some slightly strange things for Components, but for straight Element trees it "just worked" in 8 because we annotated the DOM with lots of stateful properties that were then used to render regardless of the root.

What Does work

In Preact X we can also do this, with the similar constraint that DOM nodes associated with Components will end up with two Virtual DOM trees, which is never desirable. In general, this technique is treading the boundaries of what we can allow and I do worry that it'll break in the future as we rely more on the Virtual DOM tree and less on the DOM tree for state.

Anyway, here's the variant that works in both Preact 8 and Preact X - we find the link element in the DOM, and call render() on it with a modified description to apply:

// grab a reference to that <a>:
const link = document.getElementById('link');

// here's what we want to mutate the link to look like:
const updatedLink = <a href="mailto:[email protected]">New EMail Me</a>;

// mutate the DOM link in-place to match our VDOM description:
render(updatedLink, link.parentNode, link);

This actually works fine aside from the concerns I mentioned above. It will trigger Preact's non-hydration initial diffing algorithm, which attempts to reconcile VDOM against DOM (similar to Preact 8's rendering).

What Doesn't Work

Now, here's the case that doesn't work in Preact X, and that could potentially represent a bug that could manifest itself in other documented use-cases. If we want to do "pinpoint" rendering like this, but the new VDOM tree we pass to render() has a different root element type (eg: <a> vs <span>), Preact X will not remove the previous DOM element we passed and will instead prepend the newly created tree before it. This is because render()'s replaceNode parameter was designed to allow diffing Preact trees within a parent that has other elements Preact shouldn't touch. In X this is important because it's fairly common to render multiple VDOM trees into the same parent element using createPortal() - each new tree needs to ignore other trees sharing that parent element.

So here's the version that fails in X but worked in 8:

// grab a reference to that <a>:
const link = document.getElementById('link');

// we'll try to mutate our link to be a span containing a link:
const updatedTree = (
  <span style="color: #777;">
    This is some more information
    <a href="mailto:[email protected]">New EMail Me</a>
  </span>
);

// mutate the DOM link in-place to match our VDOM description:
render(updatedTree, link.parentNode, link);

The output after rendering the above will be this:

  <div>
    before
+   <span style="color: #777;">
+     This is some more information
+     <a href="mailto:[email protected]">New EMail Me</a>
+   </span>
    <a id="link" href="mailto:[email protected]">EMail Me</a>
    after
  </div>

This is nearly correct, except that the original <a> we passed as replaceNode should have been removed since it was semantically replaced by our whole new tree.

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

5 participants