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

RSC compatibility #2923

Merged
merged 3 commits into from
Jun 3, 2024
Merged

RSC compatibility #2923

merged 3 commits into from
Jun 3, 2024

Conversation

vladmoroz
Copy link
Collaborator

Comment on lines -49 to +51
React.useEffect(() => {
if (typeof window !== 'undefined') {
(window as any)[Symbol.for('radix-ui')] = true;
}, []);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note this change, this is to allow simple primitives to be server components (like AccessibleIcon)

No context for how it's used in the original PR, but seems benign to do the assignment in render:
#853

Comment on lines -19 to +20
container?: HTMLElement | null;
container?: Element | null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corresponds to the type expected by React.createPortal

Comment on lines -23 to +27
const { container = globalThis?.document?.body, ...portalProps } = props;
const { container: containerProp, ...portalProps } = props;
const [mounted, setMounted] = React.useState(false);
useLayoutEffect(() => setMounted(true), []);
const container = containerProp || (mounted && globalThis?.document?.body);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When a container isn't provided, we must render twice to avoid hydration errors

Base automatically changed from vlad/esbuild to main June 3, 2024 09:33
@vladmoroz vladmoroz force-pushed the vlad/rsc-compat branch 3 times, most recently from 96d0a99 to 1d07a66 Compare June 3, 2024 09:37
@vladmoroz vladmoroz merged commit 7c0fd4c into main Jun 3, 2024
5 checks passed
@vladmoroz vladmoroz deleted the vlad/rsc-compat branch June 3, 2024 10:04
Comment on lines +26 to +27
useLayoutEffect(() => setMounted(true), []);
const container = containerProp || (mounted && globalThis?.document?.body);
Copy link

Choose a reason for hiding this comment

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

From an end-user point-of-view don't we introduce an extra frame as React will not render the content at first execution but after a second render cycle?

Copy link

Choose a reason for hiding this comment

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

Actually users can still pass the container to globalThis?.document?.body to drop that extra frame. Forget about my comment

radnan added a commit to radnan/optiaxiom that referenced this pull request Jul 31, 2024
radnan added a commit to radnan/optiaxiom that referenced this pull request Jul 31, 2024
radnan added a commit to optimizely-axiom/optiaxiom that referenced this pull request Jul 31, 2024
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.

3 participants