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

(experiment)(wip) Components: Abstract/namespace all related Navigator components in the root component #60926

Draft
wants to merge 11 commits into
base: trunk
Choose a base branch
from

Conversation

fullofcaffeine
Copy link
Member

@fullofcaffeine fullofcaffeine commented Apr 20, 2024

What / How

Namespace all children sub-components in the root NavigatorProvidercomponent. Also, export a Navigator alias for NavigatorProvider to make it more semantically sound. Keep exporting all individual components as-is for backwards compatibility.

Why

Closely related (children) components always used together should ideally be namespaced into the root parent component. This provides a cleaner API that's easier to use and understand.

TODO:

  • Fix types and make TS happy :)
  • Update the test
  • Update description
  • Any other API changes we should make as part of this changeset?
  • Discuss tree-shaking consequences and/or if it's relevant in this case.
  • Naming of the root provider component: discuss keeping it as Navigator (abstract away the provider part from the name, to simplify) or as NavigatorProvider or both.
@fullofcaffeine fullofcaffeine changed the title (experiment)(wip) Components: Abstract/namespace all related Navigator components in the root compoment Apr 20, 2024
@mirka mirka self-requested a review April 23, 2024 20:03
Copy link

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

@fullofcaffeine fullofcaffeine force-pushed the navigator-experiment branch 3 times, most recently from 970d466 to ee903b9 Compare May 1, 2024 13:31
Copy link

github-actions bot commented May 22, 2024

Flaky tests detected in 8a625f8.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9615299743
📝 Reported issues:

@ciampo ciampo self-assigned this Jun 21, 2024
@ciampo
Copy link
Contributor

ciampo commented Jun 21, 2024

Taking over this PR.

Looking at the pending items left in the PR:

Any other API changes we should make as part of this changeset?

That is currently being discussed in #60927

Discuss tree-shaking consequences and/or if it's relevant in this case.

I'm not sure if bundlers would be able to tree-shake different components sub-items (ie — if a consumer imports Navigator.Button, would it cause to also import the rest of the Navigator subcomponents?) (cc @youknowriad @jsnajdr )

I don't think it matters much in this scenario, because almost every subcomponent is necessary when using Navigator, but it would probably inform the broader decision on how to namespace subcomponents across the package.

Naming of the root provider component: discuss keeping it as Navigator (abstract away the provider part from the name, to simplify) or as NavigatorProvider or both.

I decided to rename the NavigatorProvider to Navigator.Root instead of Navigator, which IMO feels cleaner, more aligned with other component libraries (eg. Radix and Ariakit), and definitely simpler when it comes to typing the Navigator object. If we think this is a good strategy, we could apply it also to the rest of the "component families" in the repository?

At the same time, the considerations about tree shaking above could also represent an important factor to consider.

@jsnajdr
Copy link
Member

jsnajdr commented Jun 21, 2024

if a consumer imports Navigator.Button, would it cause to also import the rest of the Navigator subcomponents?

If you do this:

import { Navigator } from '@wordpress/components';

return <Navigator.Button />;

then the entire Navigator object, will all members, will be bundled. Bundlers can identify which exports are or are not used, but they don't attempt to undestand the internal structure of objects.

In theory, the Button can be a function that has this bound to Navigator, and the function body can refer to this.Root. It's very difficult to be sure.

@jsnajdr
Copy link
Member

jsnajdr commented Jun 21, 2024

If you plan to do some substantial rename of the components before stabilizing, you should be aware that there are plugins that attempt to be forward compatible and try to import the stable names first 🙂

Screenshot 2024-06-21 at 17 35 54
@mirka
Copy link
Member

mirka commented Jun 26, 2024

I'm not sure if bundlers would be able to tree-shake different components sub-items (ie — if a consumer imports Navigator.Button, would it cause to also import the rest of the Navigator subcomponents?)

I experimented a few months ago and confirmed that the major bundlers won't tree-shake dot notation subcomponents 🥲 Though, it's not technically impossible (e.g. rollup/rollup#2201).

In the end though, I don't think this will be a blocker for us adopting dot notation subcomponenting. We just need to be mindful of this when designing APIs for components that have a large, optional dependency in one of it subcomponents. An example that comes to mind would be a tooltip. As long as we design the API so the large dependency can be kept separate, it should be fine.

// ❌ Bundles an optional dep into an untree-shakeable subcomponent
<Foo.Bar tooltipText="foo" />

// 🟢 Keeps the optional dep "externalized"
<Tooltip text="foo">
  <Foo.Bar />
</Tooltip>

// 🟢 Or alternatively
<Foo.Bar tooltip={ <Tooltip text="foo" /> } />
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants