Components: replace console.warn with @wordpress/warning

[diegohaz]

Description

This PR implements the @wordpress/warning package introduced in #19317 on the @wordpress/components package.

How has this been tested?

npm run test-unit

Types of changes

Refactor

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[gziolo]

Nice, thanks for the follow-up work 💯

There are more places with console.warn, should we leave them unchanged or we should open another PR?

Do you have any thoughts on how to simplify the API? It looks like warning( true, ... ) is the most popular usage. Maybe it could be optional and detection would be type based? /cc @aduth

function warning( messageOrCondition, ...messages ) {
    const condition = typeof messageOrCondition === 'boolean' ? messageOrCondition : true;
    if ( typeof messageOrCondition === 'string' ) {
        messages.unshift( messageOrCondition );
    }
    ...
}

[aduth]

Do you have any thoughts on how to simplify the API? It looks like warning( true, ... ) is the most popular usage. Maybe it could be optional and detection would be type based? /cc @aduth

I think this is a really good point to highlight, if a common use of this would have us passing true. I think it's interesting in some of the specific changes here, because there is a condition associated with the warning, but because we also change the behavior in addition to log the warning, we can't pass that condition into the warning function like we'd normally expect to use it. I think @talldan's reaction would be a common sentiment of most people encountering this for the first time.

I'm not sure if that's necessarily indicative that we won't want the condition to be considered in warning. There could be an argument to be made that we shouldn't handle the condition at all in this utility, and instead assume that if it would apply only under a certain condition, that it be left to the developer to handle that as if. This does lose some of the convenience we'd have through the current signature (Edit: It also loses the benefit of stripping the condition for production environments, which is a bit more of an important factor, at least to the usefulness of this utility).

It might motivate us to consider whether we should treat the condition as at least optional, like @gziolo suggests. However, it seems like something where it could make sense to reverse the order of the arguments, which in my view would be a more natural way of handling this than overloading the function.

Something like:

function warning( message, condition = true ) {}

Worth noting: We probably want to make a decision on this point before Monday's plugin release RC, so as to avoid going through a deprecation lifecycle.

[diegohaz]

Based on my own experience using similar functions in other projects, I can say that warning( true, ... ) represents about 60% to 70% of the usage.

I've updated the code to remove the condition parameter for now (I can create a separate PR for that if it's better). We can add it later as an optional parameter if needed.

Worth noting: We probably want to make a decision on this point before Monday's plugin release RC, so as to avoid going through a deprecation lifecycle.

What's the best way to introduce "experimental" packages? Can we release things as 1.0.0-beta|alpha.0, for example?

[aduth]

I added an edit to my previous comment in highlighting that one of the other benefits of including this as an argument to the function itself is that the logic associated with the condition can be removed as part of the production minification (via the associated Babel plugin).

That being said, removing the argument would at least allow us to move forward and consider this as a future change, because either of the proposed adjustments (overloading vs. default parameter) would be backwards-compatible revisions.

What's the best way to introduce "experimental" packages? Can we release things as 1.0.0-beta|alpha.0, for example?

I think for the NPM package, it doesn't matter so much. We can choose to bump the major version to represent a breaking change. If we really wanted to consider it experimental, I would think that should be effectively represented on the 0.x.x major:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

https://semver.org/#spec-item-4

What matters more is that this would be exposed as a window global to plugin developers (wp.warning). For that, unless the global was prefixed to indicate it is experimental, we should want to oblige ourselves to backwards compatibility (for two versions in the plugin, or indefinitely once it releases a stable WordPress release).

[gziolo]

It still looks good to go. Good points raised by @aduth that it's safer to remove for now and iterate later. I like the fact that you could add the condition as part of the function call because it gets eliminated in the production mode together with the warning. Let's land this patch and work on the alternative approach separately.

What matters more is that this would be exposed as a window global to plugin developers (wp.warning). For that, unless the global was prefixed to indicate it is experimental, we should want to oblige ourselves to backwards compatibility (for two versions in the plugin, or indefinitely once it releases a stable WordPress release).

It made me think that in production, there should be no wp.warning in use for core packages because of dead code elimination. However, I suspect that it's going still to be listed as a dependency by the webpack plugin because it is processed before code is uglified. We should look into that later as well, it's not directly related to this package. Well, the production version of the package is super lightweight as far as I remember so maybe we shouldn't care at all.