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

Provide a way to keep existing markup as is (Custom Elements, Hydration) #757

Open
naltatis opened this issue Sep 13, 2018 · 22 comments
Open
Labels
enhancement New feature or request

Comments

@naltatis
Copy link

naltatis commented Sep 13, 2018

I'm looking for a way to make hyperapp respect existing markup thats inside the app and leave it untouched. My problem is quite similar to the issues @frenzzy and @gyopiazza already have pointed out (#741, #688).

Parts of my application are made up of sub-applications provided through Custom Elements. Due to browser support be can not rely on ShadowDOM, so these Custom Elements use the Light DOM to render their UI. Hyperapp is overwriting their content in every render cycle. I've tried the hydration workaround. But this is not really a solution. You are able to restore the markup, but the innerHTML gets completely rewritten so you loose all events.

I've compiled a little JSBin the shows one concrete scenario I'm having problems with:
https://jsbin.com/dogujuq/4/edit?html,js,output

Using react I was able to make this work using shouldComponentUpdate. But I currently don't see any solution how to make this happen in hyperapp.

@jorgebucaran
Copy link
Owner

@naltatis What about using Hyperapp to create your custom element instead of setting innerHTML. I did something similar with Superfine the other day when I was toying with custom elements.

@naltatis
Copy link
Author

@jorgebucaran We use custom elements as an abstraction between teams, so could be, that a team also uses hyperapp to build their feature, but when composing features through hyperapp I can't make any assumptions about the inner workings of the sub applications.

@frenzzy
Copy link
Contributor

frenzzy commented Sep 13, 2018

Looks like Hyperapp does not support Custom Elements hydration/recycling.
I think this case is different from innerHTML attribute recycling and probably simpler to implement.

Instead of adding a special shouldComponentUpdate property to VNode, Hyperapp may detect custom element and always do nothing with child nodes in this case.

But I'm not sure is there a way to detect custom elements other than searching for hyphen (-) in the element name. I'm afraid that a simple check if (nodeName.indexOf('-') !== -1) may slowdown rendering.

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

@frenzzy I don't think skipping over children of custom elements is the right approach here. You want to be able to compose the with children just like any other html node. (We want to score well on custom elements everywhere eventually)

For example you might want to do:

...
<custom-modal titleBarText="blah">
  <Foo state={state.foo}
  <p>{state.bar}</p>
</custom-modal>

I haven't really dug into the issue yet, but isn't part of the problem here that the OP isn't using Shadow DOM? If they were, I don't think this would be an issue. So maybe this isn't so much a bug as a question of browser support?

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

@naltatis I'm not entirely sure why, but I figured out that this works:

  connectedCallback() {
    setTimeout(_ => {
      console.log("example-alert connected - rendering a button");
      this.innerHTML = "<button>alert()</button>";
      this.querySelector("button").addEventListener("click", () => alert("foo"));
    }, 0)
  }

@naltatis
Copy link
Author

Yes using nativ ShadowDOM would solve my issue here, but when you have to support more than the latest browsers this is not an option. You have to use the normal DOM in this case.

As @zaceno pointed out correctly, it’s also not right to generally ignore the children of custom elements. Using ShadowDOM and Slots you need this ability to compose things.

I’m thinking of an opt in way to tell Hyperapp to skip/ignore the sub tree of a specific node. Maybe as a result of a lifecycle method.

@naltatis
Copy link
Author

naltatis commented Sep 13, 2018

@zaceno yes the setTimeout would work, but since I don’t control the code inside the custom element this is not an option. I explicitly chose that order of execution to highlight the issue. See the code comment in the first line ;)

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

@naltatis Ah ok.

You can tell hyperapp to skip the subtree of a specific node if you return the exact same instance of the virtual node, as the previous render -- i e if you memoize the h('custom-element') call.

That only works in the case you're patching though -- not the first render, with hydration, which is what your example is about.

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

I'm still not exactly sure what's going on. But I verified that this is only a problem when "hydrating" -- ie when the pre-rendered markup already contains the <example-alert> custom element. If the original markup is clean, the custom element works as intended, and keeps working through continued updates/patching (even without memoization)

Not to say that it's not a problem, but just to point out that if there is a fix for this, it's somewhere in hydration.


EDIT: It's even more specific than that! It's only a problem if the preexisting markup exactly matches the first render. Any deviation at all (even a whitespace in the markup that wouldn't affect the render) and everything works as expected. I'm still not sure how (where in the code) the button dissapears though.

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

Oooh, now I get it.

Before hyperapp does it's first render, the markup is this:

<main>
  <h1>click the button</h1>
  <example-alert>
    <button>
      alert()
    </button>
  </example-alert>
</main>

... because the custom element inserts the button into itself (yuck) when the DOM first mounts. When hyperapp does is about to do the first render, it parses the current DOM -- shown above, including the button -- into a vnode which it compares to the first result of the view function, which is a vnode corresponding to this:

<main>
  <h1>click the button</h1>
  <example-alert></example-alert>
</main>

-- no button!

So during the first render, hyperapp does exactly what it should do and removes the button (because as far as it knows, the button should no longer be rendered)

Now I also understand why shouldComponentUpdate fixes this in React. Our equivalent to shouldComponentUpdate is memoization -- but that doesn't work for hydration (because nothing is in the memo yet)

Sorry, I can't see any elegant, simple fixes for this a t m. I guess for now, if you use custom elements you can only have hydration if you also use shadow dom.

@naltatis
Copy link
Author

naltatis commented Sep 13, 2018

EDIT: It's even more specific than that! It's only a problem if the preexisting markup exactly matches the first render. Any deviation at all (even a whitespace in the markup that wouldn't affect the render) and everything works as expected.

I think I don't quite get what you are saying.

In my example the server rendered markup in the html document exactly matches the markup hyperapp would produce on first render. But the fact, that the custom element cames in earlier and modifies the DOM leads to the fact, that hyperapp removes the unexpected children of the custom element. The workaround tries to fix that by grapping the CEs innerHTML before render and refills it in later (oncreate), but looes all events while doing this.

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

@naltatis yeah, that's expected since you're not moving the actual elements with event listeners, but creating new ones to mimic the previous ones.

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

Thinking about it a little more. Maybe @frenzzy had a point about children and custom elements though... We shouldn't avoid them during patch, but perhaps we should ignore children during hydration (in the recycleElement function).

So this:

function recycleElement(element) {
    return {
      nodeName: element.nodeName.toLowerCase(),
      attributes: {},
      children: map.call(element.childNodes, function(element) {
        return element.nodeType === 3 // Node.TEXT_NODE
          ? element.nodeValue
          : recycleElement(element)
      })
    }
  }

into this:

function recycleElement(element) {
    return {
      nodeName: element.nodeName.toLowerCase(),
      attributes: {},
      children: element.tagName.indexOf('-') ? [] : map.call(element.childNodes, function(element) {
        return element.nodeType === 3 // Node.TEXT_NODE
          ? element.nodeValue
          : recycleElement(element)
      })
    }
  }

It still feels a little hacky, but it works (I tried it), and it should prevent custom elements' children (wether static in markup or dynamically generated) from being deleted on first render. On future renders it won't interfere. The only downside here is that we can't have hydration of children of custom elements, even if they were statically declared in the markup.

@zaceno
Copy link
Contributor

zaceno commented Sep 13, 2018

I know it's not helpful in this case, but I've got to say: my general feeling about using custom-elements in vdom-frameworks is that the only safe bet is to use shadow-dom. Vdom-engines expect to manage all of your dom for you and the more you interfere, the more risk of obscure bugs.

@naltatis
Copy link
Author

naltatis commented Sep 14, 2018

@zaceno I've just tried it in my project. This works perfectly an solves my issue. I had to make a small adjustion and add a > 0 check to the indexOf.

So this is the work code:

function recycleElement(element) {
    return {
      nodeName: element.nodeName.toLowerCase(),
      attributes: {},
      children: element.tagName.indexOf('-') > 0 ? [] : map.call(element.childNodes, function(element) {
        return element.nodeType === 3 // Node.TEXT_NODE
          ? element.nodeValue
          : recycleElement(element)
      })
    }
  }

To your point on vdom and shadow-dom: Yes I agree, using nativ shadow-dom would be the clean solution and avoid handling cases like this. But in the current situation where some browsers haven't even started to implement this, its necessary to reach that goal eventually.

@frenzzy
Copy link
Contributor

frenzzy commented Sep 14, 2018

Looks like server-side rendering of Custom Elements and recycling them on client-side is not a trivial task. See for example https://skatejs.netlify.com/ and their SSR docs where inline scripts does the job.

@dennisreimann
Copy link

The mentioned workaround using element.tagName.indexOf('-') > 0 fixes this issue for us as well. Any chance to get this integrated? Thanks to anyone digging deep on the cause of this issue!

@zaceno
Copy link
Contributor

zaceno commented Sep 20, 2018

@dennisreimann If I were you, I'd make a PR!

The contribution guidelines are here: https://github.com/jorgebucaran/hyperapp/blob/master/CONTRIBUTING.md

Beyond the info in there, It's also a good idea to prepare a small example in codepen or jsfiddle which demonstrates the problem being solved, and also to do some thinking about what (if anything) might be affected by this change. Does it have any downsides? Any ways it could be improved?

@TimOetting
Copy link

I am in the process of migrating the exact project that @naltatis has been working on to V2. Is there any solution in the new version to leave custom elements unchanged during hydration?

I tried to adapt the change being made in the recycleElement function to V2 but the custom elements` content still gets removed.

var recycleNode = (node) => {
  return node.nodeType === TEXT_NODE
    ? text(node.nodeValue, node)
    : createVNode(
        node.nodeName.toLowerCase(),
        EMPTY_OBJ,
        node.tagName.indexOf('-') > 0 ? [] : map.call(node.childNodes, recycleNode),
        SSR_NODE,
        node
      )}

As @jorgebucaran mentioned in #764 , the DOM mounting mechanism has been changed in V2 and mentions moving the hydration code to an external package. However, the issue is already 4 years old. I still have to sort my head out on how an external hydration package would fit in here. Would this still be the way to go for you?

@jorgebucaran
Copy link
Owner

Do you know what exactly needs to change (in a non breaking, backwards compatible way) to support this by default?

@TimOetting
Copy link

I just realized that a misunderstanding on my part caused the app to re-render my entire root node during hydration and my above attempts failed because of it. Unlike V1, Hyperapp now seems to replace the initial node with the view instead of using it as the parent node. Having this fixed, the code works so far.

I am now looking again at @naltati's PR and see if I can derive a PR for V2 from that. Currently I'm obviously still missing the isRecycling variable in the right place, but that will surely be possible somehow.

@kofifus
Copy link
Contributor

kofifus commented Feb 10, 2022

@TimOetting Hi can you please share the code you mentions works above in V2 ?

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

No branches or pull requests

7 participants