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

fix: move recursive renderChildren call after appendChild #3

Merged
merged 1 commit into from
Oct 14, 2023
Merged

fix: move recursive renderChildren call after appendChild #3

merged 1 commit into from
Oct 14, 2023

Conversation

dwirz
Copy link
Contributor

@dwirz dwirz commented Oct 9, 2023

Hi there @luwes, first of all very nice project it helped us a lot with SSR our web components within a Next.js app directory project. 🎉

What I encountered is if we have web components which use the <slot>-element within another web components <slot>-element the react renderChildren function gets somehow stucked in an infinite loop. I was able to fix it by moving the recursive call of renderChildren after the appendChild call.

Since our code is on our customers private repository i can't provide you with some reproduction code.

Dummy codewise it looks something like this:

// React JSX
return (<CustomDropdown text="Dropdown" hint="Hint" label="Label" ref={ref}>
    <CustomFlyout> // slot of custom-dropdown web component
      <CustomFlyoutItem item="item-1" label="Item 1" /> // slot of custom-flyout web component
      <CustomFlyoutItem item="item-2" label="Item 2" /> // slot of custom-flyout web component
      <CustomFlyoutItem item="item-3" label="Item 3" /> // slot of custom-flyout web component
      <CustomFlyoutItem item="item-4" label="Item 4" /> // slot of custom-flyout web component
    </CustomFlyout>
  </CustomDropdown>);

@netlify
Copy link

netlify bot commented Oct 9, 2023

Deploy Preview for wesc-eleventy canceled.

Name Link
🔨 Latest commit 0b754b5
🔍 Latest deploy log https://app.netlify.com/sites/wesc-eleventy/deploys/6523c6f39955430008658716

@vercel
Copy link

vercel bot commented Oct 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wesc-astro ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 9:26am
wesc-nextjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 9:26am
wesc-sveltekit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 9:26am

@netlify
Copy link

netlify bot commented Oct 9, 2023

Deploy Preview for wesc-node canceled.

Name Link
🔨 Latest commit 0b754b5
🔍 Latest deploy log https://app.netlify.com/sites/wesc-node/deploys/6523c6f4341e560008ccd5d2

@netlify
Copy link

netlify bot commented Oct 9, 2023

Deploy Preview for wesc-remixrun canceled.

Name Link
🔨 Latest commit 0b754b5
🔍 Latest deploy log https://app.netlify.com/sites/wesc-remixrun/deploys/6523c6f45cc1ab00087a66de

@luwes
Copy link
Owner

luwes commented Oct 9, 2023

thank you! I will try to review this week

@dwirz
Copy link
Contributor Author

dwirz commented Oct 13, 2023

Hello @luwes

it was easier for me to copy your code and adapt it to our needs in our monorepo. Since we are developing our Web Components with Stencil.js and there are two changes (#4914 and #4916) in their code that we need, I created a minimal reproduction repository.

My version of your code is here ./packages/web-components-react-wrapper/lib. The main changes are the ones I described in this PR and PR #4, see .../lib/client/render.ts#L27. Additionally, I added a renderCustomElements that renders all of our custom elements that are inside the React Wrapped Web component.

If you have any questions/ideas/feedback, let me know 🙂 .

IMO you can close this PR and the other one if you don't need my changes.

Thanks a lot for your work

@luwes
Copy link
Owner

luwes commented Oct 14, 2023

All good! I'm glad the project is useful to you.

This PR LGTM! Will merge it in.

@luwes luwes merged commit 5fe5aef into luwes:main Oct 14, 2023
18 checks passed
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.

2 participants