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

RFC: Context scoping for compound components #23287

Closed
jjenzz opened this issue Feb 13, 2022 · 7 comments
Closed

RFC: Context scoping for compound components #23287

jjenzz opened this issue Feb 13, 2022 · 7 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@jjenzz
Copy link

jjenzz commented Feb 13, 2022

The problem

Our Radix Primitives API is intentionally very open allowing for all sorts of compositions. For example:

<AlertDialog.Root>

  <Dialog.Root>
    <Dialog.Trigger />
    <Dialog.Content>
      <AlertDialog.Trigger /> {/* note the alert trigger in dialog content */}
    </Dialog.Content>
  </Dialog.Root>

  <AlertDialog.Content />
</AlertDialog.Root>

An issue arises here though...

The AlertDialog is a Dialog composition under the hood with additional functionality bound to meet AlertDialog requirements. Therefore, AlertDialog.Root is also a Dialog.Root so provides both DialogContext and AlertDialogContext.

Due to the way React Context works, the composition above means the AlertDialog.Trigger (also a Dialog.Trigger) would useContext(DialogContext) and retrieve the context from Dialog.Root instead of AlertDialog.Root. Clicking the AlertDialog.Trigger would toggle the Dialog.Content.

An initial solution might be to create hooks containing all Dialog functionality so that we can create a new context for each component type and bind dialog functionality using hook. However, if consumers create their own Dialog compositions from ours the problem still exists, it's just moved:

<FeedbackDialog.Root>
  <AnotherDialog.Root>
    <AnotherDialog.Trigger />
    <AnotherDialog.Content>
      <FeedbackDialog.Trigger /> {/* toggles AnotherDialog */}
    </AnotherDialog.Content>
  </AnotherDialog.Root>
  <FeedbackDialog.Content />
</FeedbackDialog.Root>

Instead of using hooks, we currently solve this internally by creating a new context that AlertDialog passes to its internal Dialog via a __scope prop, which Dialog then passes to all of its useContext calls. Unfortunately, this still doesn't solve the consumer side issue mentioned above.

This also sounds a lot simpler than the actual implementation ended up being because an AlertDialog can be recomposed which has its own multiple parts as well as dialog parts. Therefore, __scope is a mapping of contexts e.g:

__scope={{
  AlertDialog: [alertDialogContext, alertDialogContentContext]
  Dialog: [dialogContext, dialogContentContext]
}}

The Proposal

A way to scope a component's context when re-composing it to ensure AlertDialog parts don't accidentally retrieve context from another component composed of similar parts.

Given composition is a big part of React, it seems this would be really useful if it were handled by React somehow. Perhaps it already is and there is a simpler way we can achieve this with a different approach?

We have had someone request that we expose our scoping solution as an npm package (which we can but it needs refining before we could do that) so it seems others are facing these issues.

"Just re-order parts"

We could make sure all AlertDialog parts are inside Dialog to avoid this but a more real-world use-case where re-ordering parts is not always possible or obvious where they should go would be a DropdownMenu and Tooltip that both use a Popper under the hood, so both reference PopperContext:

<DropdownMenu>
  <Tooltip>
    <DropdownMenuTrigger as={TooltipTrigger} />
    <TooltipContent />
  </Tooltip>
  <DropdownMenuContent />
</DropdownMenu>

The popper rendered by DropdownMenuContent cannot getBoundingClientRect for its triggerRef because the ref is attached to the Tooltips PopperContext.

@jjenzz jjenzz added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Feb 13, 2022
@penx
Copy link

penx commented Feb 13, 2022

Could you allow a context to be passed in as an optional prop to Dialog.Root?

i.e. the AlertDialog primitives create their own context for Dialog and pass it in as a prop to Dialog.Root and any other Dialog primitive that may need it:

https://codesandbox.io/s/stupefied-zhukovsky-etk9m

@jjenzz
Copy link
Author

jjenzz commented Feb 13, 2022

Could you allow a context to be passed in as an optional prop to Dialog.Root?

That's what we're doing:

Instead of using hooks, we currently solve this internally by creating a new context that it passes to its internal Dialog via a __scope prop, which Dialog then passes to all of its useContext calls.

But doesn't solve it consumer-side 😢 we can suggest consumers do the same if we package our solution but given composition is such a fundamental part of React, it feels like something that should be easier to achieve.

@penx
Copy link

penx commented Apr 26, 2022

This isn't a solution, but perhaps of interest, Jotai has a concept of 'scope' that is passed to the Provider and the useAtom hook:

https://jotai.org/docs/api/core#type-script
https://github.com/pmndrs/jotai/blob/main/src/core/Provider.ts
https://github.com/pmndrs/jotai/blob/main/src/core/useAtom.ts

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
@jjenzz
Copy link
Author

jjenzz commented Apr 10, 2024

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2024
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jul 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants