-
Notifications
You must be signed in to change notification settings - Fork 2k
[DataViews]: introduce DataViewsContainer component #102987
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this is close to what I had in my, but why a new component instead of just passing the "children" prop to the DataViews
component.
- when the children prop is passed to the
DataViews
It would only render the<DataViewsContext.Provider
which it already renders - when the children prop is not passed, it would work just like today.
In other words, the DataViews
component is the provider when "children" are passed.
Yes, I know this is the approach we discussed yesterday. The issue with this “dual” behavior is that I ran into a TypeScript problem I couldn’t solve properly. If we go this route, we might introduce a breaking change at the TS level. Assuming that’s true, would it still be okay to move forward with an approach that could trigger TS errors when users update the package? Or should we consider releasing a new major version where TS compatibility is not guaranteed? |
@youknowriad this is the PR #102996 that implements what we agreed yesterday, with some notes about the TS issue. |
ad87734
to
4eb44a2
Compare
6e1d398
to
15c6d19
Compare
15c6d19
to
f4c2187
Compare
Reduces type duplication and improves maintainability
closing in favor of #102996 |
Closes WOOA7S-124
Note
This PR is one of two alternative approaches currently being explored for improving DataViews composition.
Please also see [DataViews]: Support dual rendering mode (controlled props or free composition) #102996 for the other direction we’re considering.
Introduce
DataViewsProvider
, a new wrapper component that allows free composition using the existingDataViewsContext
.Why are these changes being made?
Our goal was to enable a more flexible usage of
DataViews
, where developers can access data and shared config from context, without relying on the built-in layout rendering or manually passing props through the component tree.We initially explored supporting both "controlled" (rendered) and "headless" (composed via children) behaviors directly within the
DataViews
component.However, this approach introduced complex conditional prop logic and made type inference unstable, particularly when working with optional
children
and conditionaldata
/view
combinations.Additionally, this duality made the TypeScript definitions fragile and more complex to maintain or extend. In multiple iterations, we encountered type assertion issues, context mismatches, etc.
To reduce maintenance risk and preserve backward compatibility (both in runtime behavior and type definitions), we opted to create
DataViewsProvider
as a lightweight facade. It provides only the context layer, allowing custom rendering logic while preserving the same context shape and types.This setup also ensures we keep the
DataViews
component aligned with Gutenberg’s implementation, minimizing divergence and simplifying future upstream merges.Testing Instructions
yarn dataviews:storybook:start
DataViewsProvider
renders correctly usingFreeComposition
story.DataViews
behavior remains unchanged in all existing stories.Pre-merge Checklist