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

Web Component Complex Values not working as expected on latest canary #65584

Open
cjpillsbury opened this issue May 9, 2024 · 3 comments
Open
Labels
bug Issue was opened via the bug report template.

Comments

@cjpillsbury
Copy link

cjpillsbury commented May 9, 2024

Link to the code that reproduces this issue

https://codesandbox.io/p/devbox/jovial-panka-9slgt3

To Reproduce

  1. Load the code sandbox
  2. Note that the first web component (one simply using 'use client') never gets hydrated with the complex value.
  3. Note that the first web component (one simply using 'use client') never gets hydrated with the native onclick event handler callback, but does get the React built-in onClick (synthetic) event handler callback.
  4. Note that the second web component (one that uses dynamic loading and ssr: false) does get all complex values set via the web component's properties and new automatic event listener adding/removal.

Current vs. Expected behavior

NOTE: This is on canary for react 19 beta, so these may already be on the the team's radar, in which case feel free to close! Also feel free to reach out if any assistance with nextjs + react 19 + web components would be helpful!

Per standard hydration behavior and what I believe were the intentions of @josepharhar's work on the design here, I'd expect complex values to be populated on the client after initial load, e.g.:

Provide environment information

Operating System: (NOTE: This is not unique to a particular OS)
  Platform: linux
  Arch: x64
  Version: #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023
  Available memory (MB): 8198
  Available CPU cores: 4
Binaries: (NOTE: I'm highly confident that this is not unique to particular binaries)
  Node: 20.9.0
  npm: 9.8.1
  Yarn: 1.22.19
  pnpm: 8.10.2
Relevant Packages:
  next: 14.3.0-canary.53 // Latest available version is detected (14.3.0-canary.53).
  eslint-config-next: N/A
  react: 19.0.0-canary-fd0da3eef-20240404
  react-dom: 19.0.0-canary-fd0da3eef-20240404
  typescript: 5.1.3
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Not sure

Which stage(s) are affected? (Select all that apply)

next dev (local), next build (local), next start (local), Vercel (Deployed)

Additional context

Per callout above, I'm happy to help out with any additional questions/comments/concerns, either for this particular issue or with getting Next + React 19 + Web Components into a good spot.

@cjpillsbury cjpillsbury added the bug Issue was opened via the bug report template. label May 9, 2024
@josepharhar
Copy link

Note that the second web component (one that uses dynamic loading and ssr: false) does get all complex values set via the web component's properties and new automatic event listener adding/removal.

This has the desired outcome, right?

The current implementation of custom elements in react is only at step 1 as I wrote in my doc, so it is best to work around server rendering and hydration of custom elements as you did in the second web component because react can't know how to apply props to a not-yet-defined custom element.

This is the first feedback about server rendering and hydration I've gotten since developing the improved custom elements in react feature years ago, so this is a good signal that we should consider implementing step 2 in my doc to make it easier to do the workaround that you made in the second web component.

(I may be mis-interpreting what's going on here, I don't understand next.js very well and I didn't dive in very deep to the codesandbox)

@cjpillsbury
Copy link
Author

cjpillsbury commented May 16, 2024

All good!
Here's the tl;dr:
Yes, I do think getting farther along with the RFC's goals would be a valuable pre-requisite for accounting for different Next.js client+server permutations. The workaround certainly makes sense, and your callout that the current react implementation isn't a full implementation of your original RFC plans is helpful. I was under the impression that react 19's web component support would be closer to what was documented in the RFC. Happy to help in any way I can here (including potential PRs, where appropriate), and also would love to have some visibility/information on the plans/timelines of fully implementing the RFC goals, assuming that's feasible. I do have some concerns about not resolving facebook/react#29037 before React 19 moves from RC to official, since that would be a breaking change (not the end of the world, but may lead to some unfortunate friction).

Here's a more detailed breakdown of the situation. With respect to different client+server permutations on Next.js, we've got 3 relevant cases:

  1. No rendering, partial hydration, etc. etc. occurs on the server

Example:

const MyReactComponent = dynamic(() => import('./MyReactComponent'), {
  loading: () => <p>Loading...</p>,
  ssr: false, // This is what ensures the component will be instantiated *entirely* on the client side
});

// ...

const MyContainerComponent = () => {
  // ...
  return <MyReactComponent/>; // Using the component, including on an RSC
};
  1. Partial hydration occurs on the server, followed by client side updates
    • This is any component module flagged with 'use client';
  2. full on RSC/server-side only rendered components
    • No rendering et al. on the client.

Ideally, imo, we want all 3 permutations to work smoothly with web components for relevant use cases.

(1) should only be necessary if a web component author does not e.g. provide appropriate shims for Web APIs defined on globalThis to ensure their module does not explode when running on e.g. node (like my team does on our Media Chrome web component based library). There are other reasons, unrelated to web components, that authors may want to dynamically load, but it should not be a pre-requisite for using web components on Next.js. Currently, this is the only permutation that works reliably/predictably with Next.JS.

(2) is the most likely desired permutation for most web component + Next.js use cases. The problem with the current behavior is that something will end up being set as an attribute on the partial hydration server side, but never get set as a property on the second pass on the client side, including the cases I described in facebook/react#29037. This effectively makes partial hydration unpredictable and likely to be avoided for (almost?) all React + web component usage. I also think this is a reason to ensure things like objects are never set as attributes (per my discussion in the linked react issue).

(3) is also useful, but likely only for a subset of web components (e.g. custom container web components, display-only web components, and the like, all only if they can be useful with only attributes).

@josepharhar
Copy link

Thanks! I think I have a more comprehensive response to this issue at large in the React issue: facebook/react#29037 (comment)

I think that your example code in case 1 if fleshed out could be very valuable documentation for other people using the new custom elements in React support in next.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

2 participants