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

React fails to manage children in Stencil <slot /> #2259

Closed
bbellmyers opened this issue Mar 10, 2020 · 11 comments · Fixed by #5148
Closed

React fails to manage children in Stencil <slot /> #2259

bbellmyers opened this issue Mar 10, 2020 · 11 comments · Fixed by #5148
Labels
Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team slot-related

Comments

@bbellmyers
Copy link
Contributor

bbellmyers commented Mar 10, 2020

Stencil version:
@stencil/core@1.8.1

I'm submitting a:

[X] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://stencil-worldwide.herokuapp.com/ or https://forum.ionicframework.com/

Current behavior:
Given: one or more nested Stencil container tags with Shadow DOM off, which insert DOM levels between the Host and the <slot />.

  • component-a: Inserts a div between host and <slot />.
  • component-b: Renders its <slot /> inside component-a
    When React manages the children of the Stencil container tag, it fails to add and remove them correctly. This happens on every render after the first one.
  1. Added items are rendered outside the intended slot.
  2. Removing items throws a React error and the app fails to render.

Expected behavior:
Adding and removing items from the children array should all render in the expected slot.

Steps to reproduce:
See this github repo for an example.
React app that uses Stencil components: https://github.com/bbellmyers/stencil-slots
Stencil component library: https://github.com/bbellmyers/stencil-slot-order

Related code:

Component a:

<div class="component-a-inserted">
    <slot />
</div>

Component b:

<component-a>
    <slot />
</component-a>

React render:

<component-b>
  {children.map((child, index) => (
    <div className="example"
      key={`item-${index}`}>child {child}</div>
  ))}
</component-b>

If the children array is longer or shorter, it fails to render correctly on the second render. re-rendering with any array of the same length works.

Other information:

@ionitron-bot ionitron-bot bot added the triage label Mar 10, 2020
@max-scopp
Copy link

Just leaving notes that I had with @bbellmyers .

Using deeply nested slots don't work without ShadowDOM because hyperscript efficiently moves the existing nodes.

Afaik, we cannot tell React-DOM when an external change has been made. Possibles solutions, at least when compiling as React target, could be to make an internal HOC to render the stencil-deeply nested slot as direct children for React.

@sentience
Copy link

sentience commented Jul 9, 2020

This is looking like a potential blocker to our adoption of Stencil in our design system, which would need to service React and Elm projects (I'm currently working on an Elm output target for Stencil) and support IE11 (thus no Shadow DOM).

At this stage we're thinking maybe we can use slotted child elements:

<stencil-menu>
  <stencil-menu-item/>
  <stencil-menu-item/>
  …
</stencil-menu>

…as long as the list of menu items is stable (it's OK if the menu items themselves change – they just can’t be added/removed dynamically).

And in cases where a dynamic list of children was needed, we’d make our components support an alternative API using a render prop:

<stencil-menu children={() => [
  <stencil-menu-item/>,
  <stencil-menu-item/>,
  …
]}>
</stencil-menu>

Curious if anyone has any thoughts/experience to share about this sort of work-around.

@max-scopp
Copy link

max-scopp commented Jul 9, 2020

Hi, I’m in holidays, but we’re running a production version on our client and we didn’t encounter this problem again. We’re using React. Not sure if we had a workaround or we just used it in a way where this didn’t happen.

Can you make a repo with the exact problem you have? Using props as “children” is technically doable, but I’d highly discourage it, mainly because of the overhead needed to support/test this.

Edit: there was a different issue regarding the order and agressivity on how the components initialize, maybe related.

@sentience
Copy link

@max-scopp I'm seeing the same issue that @bbellmyers reported. I've put together a fresh demo repo with the latest versions of Stencil, react-output-target, and create-react-app, but it's essentially the same thing Bruce shared previously:

https://github.com/sentience/stencil-vdom-test

Lots of detail in the README.

@sentience
Copy link

For anyone else who encounters this, @bbellmyers has documented a pragmatic work-around that should save your bacon in the 99% case:

Stop-gap Solution

If you alter the example to add a React key attribute that is driven by the mapped children array length, then you can force a repaint of the entire container component, avoiding this failure condition (at the expense of rendering performance, unfortunately):

<component-b key={`component-b-${children.length}`}>
  {children.map((child, index) => (
    <div className="example"
      key={`item-${index}`}>child {child}</div>
  ))}
</component-b>

The main downside of this is that you will lose any private state from the component (including browser DOM state, such as keyboard focus or a text selection) when you force React to replace it.

@bbellmyers
Copy link
Contributor Author

bbellmyers commented Jul 13, 2020

React gets confused if the parent component adds any dom levels that React doesn’t know about, basically. So one way to manage is have parent component do all its work with just <Host> if it can (don’t insert any levels)

Or wrap the children in a single component that react DOES know about - a simple div will do, or in your case, maybe a <stencil-menu-item-group>?

@johnjenkins
Copy link
Contributor

johnjenkins commented Jul 20, 2020

Just chiming in to say, this isn't just a problem in react but vue too (so probably not the right place to put this?).
The 'solution' is pretty much the same too:

<component-a v-bind:key="'component-a-' + children.length">
  <div v-for="(thing, i) in children">Slotted stuff</div>
</component-a>

@nilssonja
Copy link

nilssonja commented Oct 14, 2020

Just wanted to add to the conversation with a potential solution(?) using React portals I came up with when running into the same issue. Mind you, I wouldn't call it the perfect solution to this problem but rather an alternative, perhaps.

I should preface by saying I am making use of the React bindings as one of my output targets. I am then modifying the output of the bindings by changing the components.ts file to components.tsx, and then extending the React wrappers the bindings create.

In the components.tsx file, the bindings generate a line of code to create and export a React wrapped Stencil comopnent like so:

export const ComponentName = /*@__PURE__*/createReactComponent<JSX.ComponentName, HTMLComponentNameElement>('component-name');

Since the bindings alone do not solve for this issue in React, I created something of an HOC that would take in the component to render, as well as a selector for the element inside the stencil component where things will be slotted (Which means currently this only works for components using a single <slot/>). I then rename the generated component wrapper, and instead do the following:

const ComponentNameRenamed = /*@__PURE__*/createReactComponent<JSX.ComponentName, HTMLComponentNameElement>('component-name');

export const ComponentName = withPortal<JSX.ComponentName, HTMLComponentNameElement>(ComponentNameRenamed, '.slotSelector')

I then define a simple Portal component that takes children and a reference to an element that will act as the portal. In our case, we want the elment in our Stencil component's render method that contains the <slot />:

const Portal = (props: { children: React.ReactNode; el: Element; }) => {
  return (
    ReactDOM.createPortal(props.children, props.el)
  )
}

In my withPortal hoc, I conditionally render the portal with all children passed to it once the element where children get slotted is rendered by Stencil. In essence this effectively bypasses Stencil moving the children via and instead tells React to insert the children where they need to go.

The implementation for withPortal is as follows (scrutiny very much welcome as my typescript skills are quite new 😅):

const withPortal = <
  PropType,
  ElementType extends HTMLStencilElement
>(Component: ReactType, slotSelector: string) => {
  return React.forwardRef(({children, ...props}: StencilReactExternalProps<PropType, ElementType>, ref: Ref<ElementType>) => {
    const hostElementRef: Ref<ElementType> = ref && "current" in ref ? ref : React.createRef();
    const [slotRef, setChildRef] = useState(null)

    React.useEffect(() => {
      const node = hostElementRef.current;

      const selectChildElement = () => {
        const childElement = node.querySelector(slotSelector)
        setChildRef(childElement)
      }

      if(ref && !("current" in ref)) {
        ref(hostElementRef.current);
      }

      node.componentOnReady ? 
        node.componentOnReady().then(selectChildElement)
        : node.addEventListener('ready', selectChildElement)
    }, []);
    return (
    <Component
      ref={hostElementRef}
      {...props}>
        {slotRef && <Portal el={slotRef}>{children}</Portal>}
    </Component>
  )})
}

All in all what you are left with is a React component that bypasses Stencil's handling of slotted children, allowing React to handle inserting the children itself via a Portal. I think one of the biggest pros to this solution is not having to re-render the entire component on every addition/removal of children. However, it would require additional tweaking for more complex components that use named slots. Additionally the use of React portals may be a hindrance if consumers of your component library are using SSR.

Copy link

ionitron-bot bot commented Dec 2, 2023

Thanks for the issue! This issue is being closed due to inactivity. If this is still an issue with the latest version of Stencil, please create a new issue and ensure the template is fully filled out.

Thank you for using Stencil!

@ionitron-bot ionitron-bot bot closed this as completed Dec 2, 2023
@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Dec 2, 2023
@rwaskiewicz rwaskiewicz added Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team and removed ionitron: stale issue This issue has not seen any activity for a long period of time labels Dec 4, 2023
@rwaskiewicz
Copy link
Member

Reopening - this shouldn't have been closed. Sorry about that!

@rwaskiewicz rwaskiewicz reopened this Dec 4, 2023
@tanner-reits tanner-reits linked a pull request Dec 12, 2023 that will close this issue
2 tasks
@rwaskiewicz
Copy link
Member

The fix for this issue has been released as a part of today's Stencil 4.9.0 release under the experimentalSlotFixes flag (docs).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team slot-related
Projects
None yet
6 participants