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

query and queryAll don't work in the shadow dom #2

Open
semmel opened this issue Oct 11, 2019 · 6 comments
Open

query and queryAll don't work in the shadow dom #2

semmel opened this issue Oct 11, 2019 · 6 comments

Comments

@semmel
Copy link

semmel commented Oct 11, 2019

in lib/define.js:

query: query(this), // <-- bound to the element not element.shadowRoot
queryAll: queryAll(this)

Although it's easy to just element.shadowRoot.querySelector(sel), I might be missing something here? It seems to me that shadow dom is one of the key benefits of web components.

Also just out of curiosity, why do query and queryAll return Promises? It's not that I could not handle it, but I don't find any indicator why this is necessary.

Btw. I really appreciate the osagai's functional approach - it makes all that class noise go away 😄. Also like to compliment you on the beautiful documentation - it really shines 🌻 !

@HenriqueLimas
Copy link
Owner

@semmel thank you very much for pointing this out and really sorry for the delay.
Answering your questions, the reason that the query and queryAll are promises is that because at the time that the Osagai component is created, the element is not rendered yet. If it wasn't a Promise, calling query('some-child') will return null because the child is not rendered yet.

And you are right. I will update the querys functions to support the shadow dom. 👍

@HenriqueLimas
Copy link
Owner

HenriqueLimas commented Nov 2, 2019

@semmel, version 0.3.3 added shadow root support on both functions. You can check the commit here 028f8be

Let me know if it fix the problem. 👍

@semmel
Copy link
Author

semmel commented Nov 4, 2019

Despite the nice refactoring and inclusion of shadow dom for query I still encounter some problems. Fortunately all can be solved quite easy with osagai:

  1. With shadow dom especially the custom element does not need a single parent node. Instead it's a collection of nodes. E.g. the html in the hello example could become:
`<button class="btn" id="onlyButton">Click me</button>
    ${state === "loading" ? "Loading..." : ""}
    ${state === "loaded"
      ? `<ul class="list">
        ${items.map(item => `<li>${item.name}</li>`).join("")}
      </ul>`
    : ""
  }`

This however does not work well when morphdom receives the target as html string. (Try this dom in the example to see it break.) The solution is to specify the target as document fragment instead of a string:

const templateElement = document.createElement("template");
templateElement.innerHTML = templateHTML;
morphdom(
  element.__shadowDom__,
  templateElement.content
);

At first this seems to work when patching lib/core/updateDom. With the integrated outdated version of morphdom however it needlessly destroys nodes. The latest version of morphdom fixes this. Fortunately it can be given as custom renderer to osagai's define. 😃

It seems morphdom continuously receives bug fixes, so why not either

  • require it as a dependency to osagai, or
  • replace it with a dumb algorithm and leave the dom diffing renderer entirely to the user?
  1. the reason that the query and queryAll are promises is that because at the time that the Osagai component is created, the element is not rendered yet. If it wasn't a Promise, calling query('some-child') will return null because the child is not rendered yet.

Well imo query is not a very "smart"-delay promise: It just puts itself in the JS job queue which (running inside a JS message queue cycle) not responding to any web component lifecycle event or any other callback. I am surprised to learn that this schedule guarantees rendering of the custom element's dom (regarding all morhpdom diffing steps) in a timely manner...

@HenriqueLimas
Copy link
Owner

HenriqueLimas commented Nov 10, 2019

Thank you @semmel for finding this out. I added support for multi child on shadow dom on the latest version (0.3.5) and update morphdom with latest version.

The reason osagai has a "forked" version of morphdom is because we need to add a check to not consider the diffing for custom elements tags. This is because the way the diff works on morphdom, the constructor of the custom element is executed every time we made the diff (this is true only for elements that doesn't use shadow dom). But this is not expected on osagai word, since we could add event listeners or async calls in the constructor.

And yes, query are not so smart at all. But I didn't want to add some extra logic for now since I never found any issue with it. As soon as this could be a problem we could find a better way of doing it. UPDATE: The todomvc on the demo folder has this problem and the work around was to add a global click on the element it self. I will figure out a better way for the query and queryAll functions

@semmel
Copy link
Author

semmel commented Nov 10, 2019

@HenriqueLimas, I very much appreciate the work you put in fixing and testing my issue with the multi-child shadow-dom elements! It's really great to see the development of this valuable library (great testing and nice functional API surface) 💫

The reason osagai has a "forked" version of morphdom is because we need to add a check to not consider the diffing for custom elements tags.

I did not know that osagai already does this. Great! But of course as you say it's necessary for this library.
Up to now I excluded child custom elements from diffing using, as I mentioned, morphdom as custom renderer with it's onBeforeElUpdated hook option. I guess that I can now ditch morphdom as custom renderer completely.

Regarding query's behaviour, it's not such a pain point right now, because I shadowRoot.querySelector particular child elements not before the onConnected lifecycle event where I also register event handlers and stuff. (Of course keeping fingers crossed that no dom update destroys them.) So it is just an observation, that this query is offered by osagai and thus should work comprehensible.

@semmel semmel closed this as completed Nov 19, 2019
@semmel
Copy link
Author

semmel commented Nov 19, 2019

The reason osagai has a "forked" version of morphdom is because we need to add a check to not consider the diffing for custom elements tags. This is because the way the diff works on morphdom, the constructor of the custom element is executed every time we made the diff (this is true only for elements that doesn't use shadow dom). But this is not expected on osagai word, since we could add event listeners or async calls in the constructor.

Unfortunately I've found that this causes osagai to ignore child web components with Shadow DOM Slots. Such web components not only update via attributes, but also via Light DOM which is kind of an external child of the web component.

With the untweaked morphdom (or commenting out your isCustomElement in osagai/dom.js) osagai works just great with such elements.

As a demo I created a (rather playful) example with such a Light DOM template for the host web component (my-garage):

<my-garage>
<my-car ${car.isEngineRunning ? 'isrunning' : ''}><span>${car.picture}</span></my-car>
</my-garage>

See this working Codepen example with Morphdom as custom renderer and really no extra work.

This codepen uses the same code with osagai's internal forked morphdom renderer. But here only the attributes of the <my-car> get updated, <my-car>'s Light DOM is completely ignored.

If my observations are correct, in my case with Shadow Dom web components the inlined version of morphdom is useless to me. All I need is the customRenderer option with the template element as intermediate step.

@semmel semmel reopened this Nov 19, 2019
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

2 participants