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

Allow static children in shadow: false mode #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jvdsande
Copy link

@jvdsande jvdsande commented Jan 6, 2021

What does this PR do

This PR allows passing children for elements even with shadow: false. It only works for first render, if the children are modified after first render (i.e. if the element is used in a framework such as LitElement), children are not updated.

It also cleans up the overall usage with shadow: false by getting rid of the <slot> wrapper element, attaching the context event listener on the custom element itself.

Why is this PR needed

It partly fixes #41.
Children are a big part of a component design, not being able to pass children is a strong limitation. As of now, passing children results in them being duplicated. With this PR, at least the initial rendering is correct. It also enables internally handled children to be hydrated when using SSR.

Taking the following example:

function MyComponent({ children }) {
  return (
    <div class="children-go-here">
       {children}
    </div>
  )
}

register(MyComponent, 'my-component', [], { shadow: false })
<my-component>Hello World!</my-component>

The resulting markup without this PR is:

<my-component>
   <!-- correctly rendered component -->
   <div class="children-go-here">
      Hello World!
   </div>
   <!-- incorrectly kept initial children -->
   Hello World!
</my-component>

With this PR, it becomes:

<my-component>
   <!-- correctly rendered component -->
   <div class="children-go-here">
      Hello World!
   </div>
   <!-- correctly cleaned-up initial children -->
</my-component>

Implementation details

This PR touches at various points of the wrapper's lifecycle. The first breaking change here is the addition of the following in toVdom:

	// Remove all children from the topmost node in non-shadow mode
	if (!shadow && nodeName) {
		element.innerHTML = '';
	}

This snippet removes all children present in the original element before rendering (and after having copied them to the vDom's children), thus removing the duplicated/wrongly placed children.

The PR also removes the manual call to connectedCallback when handling props update before initial render. In place, it stores the props in the _props temporary variable, so that they are applied at first render. This allows to avoid a bug where the component is rendered inside itself when connectedCallback is called multiple time.

The rest is implementation details about removing the <slot> element when not using ShadowDOM, has it is not needed.

Misc

Removing the <slot> wrapper when not using ShadowDOM allows for a cleaner DOM, but it does come at a cost: we loose the ability to unregister the _preact event listener. We could add this ability back by using a useEffect hook in the Slot component, but I did not want to add a dependency on preact/hooks there, since the library seems to avoid it.

The usage is also a bit strange since modifying the children of the custom elements will not update the children of the Component, since it does not call attributeChangedCallback. I've not found a way around this.

Finally, maybe handling children with non ShadowDOM is not something you want to be allowed, in which case this PR can just be closed 🙂

Copy link

@Vanillabacke Vanillabacke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as it should

Vanillabacke pushed a commit to Vanillabacke/preact-custom-element that referenced this pull request Apr 27, 2021
Author: jvdsande
Date: 6. Jan 2021
Reference: preactjs#56
{
name,
shadow,
addContextListener(listener, element = cn[i]) {
Copy link

@tillsc tillsc Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work as expected. When called via addContextListener(this._listener); (see Line 148) element is undefined.
I fixed this by defining a const childElement = cn[i]; at the beginning of the for loop and using childElement instead of cn[i]. So in this case the line would be:

addContextListener(listener, element = childElement) {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see this seems to be a transpiler (in my case rollup) bug...

Rollup transpiles from

let i = 0;
for (i = 1; i--;) {
  do_something_with(cn[i])
}

to code like

var i = 0;
function _loop() {
  do_something_with(cn[i])
}
for (i = 1; i--;) {
  loop();
}

Which doesn't make any sense....

addContextListener(listener, element = cn[i]) {
element.addEventListener('_preact', listener);
},
removeContextListener(listener, element = cn[i]) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Problem here as in 148

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

Successfully merging this pull request may close these issues.

"Children" not replaced on mount when shadow DOM is disabled
4 participants