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

Conflicts between bundled WordPress packages and private APIs opt-in strings. #63657

Open
youknowriad opened this issue Jul 17, 2024 · 16 comments
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] Private APIs /packages/private-apis [Type] Bug An existing feature does not function as intended

Comments

@youknowriad
Copy link
Contributor

youknowriad commented Jul 17, 2024

Description

If a plugin uses an old WordPress package like @wordpress/dataviews 2.1 with WordPress 6.6, you may see the following error:

You tried to opt-in to unstable APIs without confirming you know the consequences. This feature is only for JavaScript modules shipped with WordPress core. Please do not use it in plugins and themes as the unstable APIs will removed without a warning. If you ignore this error and depend on unstable features, your product will inevitably break on the next WordPress release.

This issue is prominent when using "bundled" packages like dataviews, icons, interface for instance as it's the way to use these packages in your plugins is to bundle them in your own code. But the reality is that the issue can actually happen with any package. For instance, I know that some plugins bundled the @wordpress/components package instead of using wp.components (they have their reasons).

The root of the issue is that our npm packages can rely on private APIs from other packages that are distributed in WordPress. In the first example above dataviews package actually uses private APIs from components to name just one and to opt-in to private APIs from other packages, you have to call something like

__dangerousOptInToUnstableAPIsOnlyForCoreModules(
		'I acknowledge private features are not for use in themes or plugins and doing so will break in the next version of WordPress.',
		'@wordpress/dataviews'
	)

The problem is that the "string" that you pass to this function to opt-in into private APIs is subject to change over time. We actually have a guideline to update this string on each WordPress release.

For the moment, I only see one solution:

  • Stop updating that string (as it can be considered a breaking change per the reasoning above)

Let me know if you have other ideas.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Package] Private APIs /packages/private-apis [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Jul 17, 2024
@peterwilsoncc
Copy link
Contributor

Could these packages be included as wp-script dependencies and the build script modified to account for the need to include them in plugins rather than via the wp global?

It's possible that the dependency version constraints would need to be modified to account for this, see discussion #52814, as the constraint settings can cause a mismatch when installing packages.

I'd rather avoid never updating the private API opt-in string as I think it could result in the old problem - the private APIs would end up being used despite the opt-in string an end up become faux stable.

If the wp-scripts pathe isn't an option then I think a linting error preventing the usee of private APIs in these types of packages could be an option.

@youknowriad
Copy link
Contributor Author

Could these packages be included as wp-script dependencies and the build script modified to account for the need to include them in plugins rather than via the wp global?

I'm not sure I understand, that's the case here these packages are included within the plugin scripts and that's what causing the problems because those scripts contain old "strings" where the WP string has been updated.

I'd rather avoid never updating the private API opt-in string as I think it could result in the old problem - the private APIs would end up being used despite the opt-in string an end up become faux stable.

I understand and I would agree, but I think the problem at hand creates way more backward compatibility issues than "not updating the string". I'd love to find other alternatives, but can't think of any at the moment.

If the wp-scripts pathe isn't an option then I think a linting error preventing the usee of private APIs in these types of packages could be an option.

That doesn't seem like an option to me, because it's an issue that can happen for all packages, there's no distinction. It just happens at the moment a plugin author decides to bundle a core package within its code.

@pdewouters
Copy link

This error appears if I upgrade the dataviews package to anything higher than 2.1.0, even on WordPress 6.5.5.

@youknowriad
Copy link
Contributor Author

@pdewouters yes, unfortunately, there's nothing we can do for previous versions now.

  • It means, that for 6.5, you can only use DataViews before 2.2
  • and for 6.6, you can only use DataViews after 2.2

We should make sure to avoid this in the future though and any dataviews (or other package relying on private apis) version can be used both with 6.6 and WP > 6.6.

@peterwilsoncc
Copy link
Contributor

I think I get it: dataviews and icons are intended to be included with plugins rather than enqueued via a version included in WP Core. Is that correct?

I'm of the view that including a bundled package from one version of WP (components in the example above) and expecting them to work with packages from different versions of WP are operating out of warranty. The packages are a collection intended to work together. The JSX changes in WP 6.6 is an example of why this can't be gaurented.

I'd like to know more about the use case of including components or other packages from a particular version of WP, though. If it's required for a reason then I may change my mind.

@youknowriad
Copy link
Contributor Author

I know WooCommerce used to bundle the components package. I'm not sure exactly about the reasons...

@peterwilsoncc
Copy link
Contributor

@youknowriad I've been thinking about how to approach this and the best approach I could come up with was in two stages

  1. Packages designed to be bundled in plugins
    a) Remove the use of private APIs from these packages for forward compatibility
    b) Create a custom eslint sniff targeting said packages to avoid the issue in the future
  2. Accounting for plugins replacing core packages
    a) Kick the decision as to whether we declare this out of warranty down the road
    b) Refrain from updating the opt-in string in 6.7
    c) A make post during the 6.7 cycle if it's decided it's out of warranty

cc @noisysocks as we discussed this via DM.

@youknowriad
Copy link
Contributor Author

Packages designed to be bundled in plugins

That feels like a user decision for me and not a developer's decision. If I use a package through npm, I should expect it to work regardless of how it's built internally. It is generally the case for all our packages though. The problem is that we also ship wp-scripts that bundle some packages but not their dependencies. So you end-up with a bundled "dataviews" package that calls a global wp.components. Maybe the right solution is to update wp-scripts to bundle these packages and their dependencies as well. And each time someone wants to use a package as "bundled", he has to ensure its dependencies are "bundled" as well.

The downside/lint rule/constraint is that you can't use bundled packages in WordPress unless you're certain that these packages don't rely on singletons (like stores...) because otherwise you end up with duplication. The other downside is the bundle size, we'll probably end up with the same component multiple times in the page for instance.

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Aug 16, 2024

That feels like a user decision for me and not a developer's decision. If I use a package through npm, I should expect it to work regardless of how it's built internally.

Yes, that is what I am suggesting, you mentioned that icons and some other packages are designed to be included in plugins rather than registered in WP.

The effect of the decision is that in order to meet the expectation that they work it prevents WordPress contributors from using the private APIs in those packages.

@adamziel
Copy link
Contributor

adamziel commented Sep 17, 2024

What's the use-case for bundling @wordpress/dataviews vs relying on the one shipped with WordPress? Private APIs may be just the canary here. Wouldn't bundling older libraries lead to loading multiple version of WordPress packages, data transfer overhead, version conflicts between the packages, Gutenberg plugin, WordPress core itself etc.?

@youknowriad
Copy link
Contributor Author

@wordpress/dataviews is far from ready to be an API that is supported in Core without breaking changes.

@adamziel
Copy link
Contributor

adamziel commented Sep 19, 2024

Aha, thank you for clarifying. Here's my understanding of the situation:

  • @wordpress/dataviews are meant for Core once the API matures
  • Today it's shipped with the Gutenberg plugin and as an npm package
  • Part of maturing the API is getting feedback from the developers using dataviews in their projects
  • Expecting every dataviews-reliant plugin to depend on a specific version of Gutenberg is a big ask
  • Therefore, developers consume dataviews through npm, and with it they bring in all the npm dependencies, such as @wordpress/data
  • These plugins are built with @wordpress/scripts, which replaces the npm version of @wordpress/data with wp.data, but it cannot do the same for dataviews because there's no core version of dataviews

Does that sound right? If yes, that last steps effectively replaces the dependency graph used by @wordpress/dataviews. The assumption seems to be "I'm sure window.wp is compatible with all possible versions of this dependency". Is that assumption correct? Aside of the private APIs, that is.

One solution would be for all the dataviews-reliant plugins to require the Gutenberg plugin after all. Then, instead of bundling the package we could perform the substitution to window.wp.dataViews and keep a coherent dependency graph.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 24, 2024

The bundled @wordpress/dataviews package has another related problem: if two different plugins bundle their own copy of @wordpress/dataviews, then one of them will fail to opt-in to private APIs. Because both copies opt-in as @wordpress/dataviews, the second one will get an error:

You tried to opt-in to unstable APIs as module "@wordpress/dataviews" which is already registered.

That happens in Core, the Gutenberg plugin does not have this double registration check.

Also, I think this discussion is focusing too much on the consent string. The real problem is that the DataViews package uses the DropdownMenuV2 private component from @wordpress/components. What if that component changes between WordPress 6.6 and 6.7? Then your plugin breaks. The consent string merely preempts that possibly subtle and hidden failure by creating a big and very visible failure instead. Kind of a "chaos monkey" approach where failures are created on purpose to find bugs faster.

@youknowriad
Copy link
Contributor Author

Yes, more and more, I'm starting to wonder whether encouraging people to actually use import something form @wordpress/something is the right thing (and our dependency extraction config). We're hiding the fact that this is not an npm package install, it's instead a wp.global usage.

It seems this obfuscation for us (core) is helpful because we actually ship npm packages that work together as regular npm dependencies do, but consumers are not.

Maybe my ideal at this point would be to:

  • Try to make sure webpack-dependency-extraction-plugin works with global variables too (Extract wp script dependencies based on global variable usage and not npm imports)
  • Remove the "externals" config from wp-scripts. If you do import something from '@wordpress/something, you're actually bundling the package just like any other bundler.
  • Some people like to use imports because they provide them with "typescript types" for their plugins. There could be something we could add to support this automatically in wp-scripts even if you use globals.

Obviously this solution is a departure from how people are used to building scripts. So maybe it's too late for it? or maybe there's a way to provide a codemod for everyone or something.

@pdewouters
Copy link

I'm starting to wonder whether encouraging people to actually use import something form @wordpress/something is the right thing

What would be a better approach for using WP packages in a plugin @youknowriad?

@anomiex
Copy link
Contributor

anomiex commented Nov 2, 2024

Some people like to use imports because they provide them with "typescript types" for their plugins. There could be something we could add to support this automatically in wp-scripts even if you use globals.

The imports also make it reasonable to do not-strictly-unit testing without going all the way to E2Es. If globals are used directly instead of imports, Jest would have to be configured with all the necessary globals filled in with appropriate implementations/mocks. So if you do go that route, please provide some package that can be loaded into a Jest configuration to set the necessary globals.

P.S. And please make it all possible to do without being forced to use wp-scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] Private APIs /packages/private-apis [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants