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.

• Required label: Any label starting with [Type].
• Labels found: [Package] Theme.

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.

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: ObliviousHarmony <obliviousharmony@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

• Within WordPress, we could get rid of the private API altogether and always use @wordpress/theme/theme-provider, which would at least avoid multiple copies in the WordPress context.

I think we could do this now. It also simplifies the code a fair bit. Updated in f974531 .

@aduth looks like /storybook/decorators/with-design-system-theme.tsx also needs updating

@aduth looks like /storybook/decorators/with-design-system-theme.tsx also needs updating

Whoops, yep 👍 Fixed in 43f13c7.

Does exposing the /theme-provider export basically prevent us from making any breaking changes to it in the future?

Can we use internal, relative paths for the time being?

Does exposing the /theme-provider export basically prevent us from making any breaking changes to it in the future?

Considering that it's only accessible to npm packages, I am expecting we could use the same npm versioning approach we're adopting for @wordpress/ui to consider breaking changes.

Size Change: +2 B (0%)

Total Size: 3 MB

_{compressed-size-action}

If we can introduce breaking changes, then I don't see the issue with using it as a private API?

I don't remember who from @WordPress/gutenberg-components I spoke with about this, but I do remember reaching the conclusion that we couldn't introduce breaking changes. I'm probably confused with another conversation.

If we can introduce breaking changes, then I don't see the issue with using it as a private API?

I might not be following, but the problem with private APIs isn't to do with change management, it has to do with multiple copies of @wordpress/private-apis conflicting with each other when @wordpress/ui components are bundled into a downstream package.

I tested this PR out by consuming @wordpress/ui via woocommerce/woocommerce#63165. I added the @wordpress/theme/design-tokens.css styles and the <Tab> component in that PR works the same as it does in Storybook for this pull request.

Great work @aduth!

If we can introduce breaking changes, then I don't see the issue with using it as a private API?

Just to quickly summarize, as is, the bundling of the UI package has questionable utility.

The problem is that using a private API in @wordpress/ui means that it depends on the wp.privateAPIs global. This wouldn't necessarily matter but @wordpress/ui was only added to @wordpress/private-apis a few weeks ago. As a result, unless you are running Gutenberg 22.5.0 or later, this line throws in __dangerousOptInToUnstableAPIsOnlyForCoreModules because CORE_MODULES_USING_PRIVATE_APIS didn't include @wordpress/ui until just now.

I think this really highlights the risk of using private APIs in bundled packages in the first place. As a downstream consumer we are basically being silently opted-in to unstable internal APIs that might change and break our code without warning, despite the package we're bundling having a stable version constraint. Realistically speaking, any time you do this you're effectively making the API public in that it's called by code bundled in the consuming app rather than WordPress.

I’d like to clarify my reasoning in case my previous explanation was unclear.

I initially believed that the @wordpress/theme package was not a bundled package, meaning we would need to be cautious about introducing breaking changes in the future, as is typically the case with most @wordpress/* packages. If this is true, exposing the ThemeProvider component as a public API now, even under the /theme-provider subpath, would limit our ability to make breaking changes later. Given how early and relatively untested this component is, I anticipate that there may indeed be breaking changes.

On the other hand, if @wordpress/theme is a bundled package, we would be able to introduce breaking changes and utilize npm's semantic versioning appropriately. In that case, I don't see a need for a dedicated /theme-provider subpath; we could simply expect users to import it directly from @wordpress/theme.

To summarize:

• If we can use npm versioning and introduce breaking changes, we might be able to export the ThemeProvider component without subpaths.
• If we cannot use npm versioning and introduce breaking changes, we should avoid adding public APIs to the theme package and temporarily import ThemeProvider using relative paths.

I hope this makes things clearer!

If this is true, exposing the ThemeProvider component as a public API now, even under the /theme-provider subpath, would limit our ability to make breaking changes later.

I don't agree that /theme-provider subpath would be subject to the same backwards-compatibility restrictions, since it's only consumable via NPM bundling and not the wp. singular global, and in that way works the same as how we expect @wordpress/ui to work.

Although I think it does raise a valid point that if we're not actually shipping anything on the "built" wp. global`, then it might as well not be a bundled package at all.

There's a couple problems with the changes I made in #75352 (comment) :

• The build script doesn't really support using subpath exports from a package that's marked as wpScript: true. I think this is what's resulting in the errors on the Fonts page seen in the E2E tests. It probably should be able to support this, but...
• We probably still want the built, private APIs. Since this ensures a single (albeit private) copy, at least in how the theme package is used within Gutenberg. It's still technically an issue within @wordpress/ui, but the usage there is more isolated that it feels less risky. Going the npm route might be possible if we could guarantee that usage was consolidated within @wordpress/boot, but I don't know that we can make that assumption.

With that in mind, I think I'm going to revert back to the original implementation which only updates @wordpress/ui to use the subpath, and restore the private APIs to the theme package and its usage in the boot module.

With that in mind, I think I'm going to revert back to the original implementation which only updates @wordpress/ui to use the subpath, and restore the private APIs to the theme package and its usage in the boot module.

Given that subpaths are not added to the global wp. object and can only be consumed via npm, this feels like the best short-term solution, even if it introduces potentially issues with duplicate React Context copies.

The long-term "fixes" are, IMO:

• understand if theme can be un-bundled, like ui and dataviews, which would allow us to use npm versioning;
• otherwise, the only "real" fix is to work towards ironing out as many potential future-compat issues as possible (to the best of our knowledge) until we feel comfortable marking ThemeProvider as a public API (although that's easier said than done)

Yeah I think we're on the same page @ciampo 👍

The changes on the branch currently should reflect that, if you're able to give it a look.

I thought ThemeProvider needed to be on the wp global because it needs to share a context object? Nesting wouldn't work correctly across instances if it were bundled.

I thought ThemeProvider needed to be on the wp global because it needs to share a context object? Nesting wouldn't work correctly across instances if it were bundled.

Right, though my expectation is that with the current usage of ThemeProvider in @wordpress/ui, this wouldn't really be an issue. Although I'd welcome someone to double-check that assessment.

Granted, I agree this feels entirely fragile, with the main point of trying to land something now to unblock others from using @wordpress/ui as it exists today.

I wonder what appetite we'd have for trying to promote ThemeProvider to a public API shortly after the 7.0 beta cut-off that's happening next week?

my expectation is that with the current usage of ThemeProvider in @wordpress/ui, this wouldn't really be an issue

It would be fine as long as all the ThemeProviders in a given app are the same version and built into a single bundle. Otherwise, the most probable failure case I can think of is:

1. The app has a ThemeProvider at the top level, setting the user's chosen accent color as primary. Background color is light by default.
2. There is a subsection of the app where there's another ThemeProvider setting the background color to dark.
3. The dark subsection of the app contains a Select component that doesn't share ThemeProvider context identity with the ThemeProvider in 2, so the Select popover ends up having a light background.

I wonder what appetite we'd have for trying to promote ThemeProvider to a public API shortly after the 7.0 beta cut-off that's happening next week?

I'll defer to the main folks who worked on it, but I'm actually more concerned about the stability of the design tokens themselves 😅 Breaking changes like #75054 are going to require back compat tokens to support older versions of @wordpress/ui (not to mention third-party usage of the tokens), once we officially make the token stylesheet public.

After some more consideration, I think this approach feels pretty untenable. Packages which use components like Dialog would not have user preferences respected on inner content in a WordPress context because it would bundle its own copy of ThemeProvider and not be able to share with the global copy.

Instead, I think we need to move toward stabilizing the ThemeProvider API so we can consolidate on wp.theme.ThemeProvider and rely on downstream consumers to use dependency extraction tools (e.g. @wordpress/scripts, @wordpress/build, or the Webpack plugin) to ensure that their code points @wordpress/theme to wp.theme globals to ensure context is shared.

We should be able to aim to have this happen shortly after the 7.0 beta cut-off so that it lands early in the WordPress 7.1 release cycle.

Instead, I think we need to move toward stabilizing the ThemeProvider API so we can consolidate on wp.theme.ThemeProvider and rely on downstream consumers to use dependency extraction tools (e.g. @wordpress/scripts, @wordpress/build, or the Webpack plugin) to ensure that their code points @wordpress/theme to wp.theme globals to ensure context is shared.

Tracking at #76840 and closing this.