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

Introduction of __next40pxDefaultSize promises BC break in WordPress Core. #55401

Closed
peterwilsoncc opened this issue Oct 17, 2023 · 26 comments · Fixed by #58703
Closed

Introduction of __next40pxDefaultSize promises BC break in WordPress Core. #55401

peterwilsoncc opened this issue Oct 17, 2023 · 26 comments · Fixed by #58703
Assignees
Labels
Needs Testing Needs further testing to be confirmed. [Package] Components /packages/components [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended

Comments

@peterwilsoncc
Copy link
Contributor

Description

The Editor Components updates in WordPress 6.4 dev-note documents the introduction of the temporary property __next40pxDefaultSize and notes that the "the temporary prop will be deprecated and ultimately removed".

Introducing a promise to break backward compatibility goes against the WordPress goal to strive to never break backward compatibility.

Furthermore, __next36pxDefaultSize has previously being introduced and now adopts the 40 pixel default size. This demonstrates that the pattern is problematic and should be avoided.

The __next prefix appears to be experimental by another name and appears to go against all the work that went in to the Private APIs package.

A consistent UI is a good aim to have but I'd like to see another approach taken to this change.

cc @hellofromtonya -- I've opened the ticket as discussed.

Step-by-step reproduction instructions

N/A

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 6.4

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@jordesign jordesign added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Package] Components /packages/components labels Oct 17, 2023
@peterwilsoncc peterwilsoncc changed the title Introduction to __next40pxDefaultSize promises BC break in WordPress Core. Introduction of __next40pxDefaultSize promises BC break in WordPress Core. Oct 17, 2023
@tyxla
Copy link
Member

tyxla commented Oct 18, 2023

Thanks for raising a concern, @peterwilsoncc!

From my understanding, the props in question at some point just start being the defaults and are unnecessary, at which point it's all the same if we pass them or not.

I may be lacking some context, but I personally don't understand how in practice this breaks backward compatibility. Could you please elaborate?

@peterwilsoncc
Copy link
Contributor Author

I may be lacking some context, but I personally don't understand how in practice this breaks backward compatibility. Could you please elaborate?

The Dev note says it's a

temporary prop will be deprecated and ultimately removed, causing all instances of the components to switch to the default 40px size out of the box.

Introducing a property with the intention of removing it says to me "your code will work as you expect now, but we intend to break it".

I don't know what the better option is but with __next36pxDefaultSize now being an alias for __next40pxDefaultSize we've got a history of the practice of using these temporary props not working so doubling down on it seems flawed at best.

@tyxla
Copy link
Member

tyxla commented Oct 19, 2023

Introducing a property with the intention of removing it says to me "your code will work as you expect now, but we intend to break it".

Thanks, I understand where the confusion comes from. I can confirm that removing the prop will not change/break anything.

The intention to introduce it in the first place is to only allow for consumers to opt for the next default size earlier if they wish. In a way, it's helping with the adoption of the new sizes, and serves as a lever to improve compatibility, and not as a backward compatibility breaker.

I guess the confusion comes from the vocabulary choice - do you have any tips for improving that wording to ensure no backward compatibility will be broken by removing the prop @peterwilsoncc @bph @hellofromtonya?

@peterwilsoncc
Copy link
Contributor Author

@tyxla If something like this must be done, can an HTML class be used instead? It's more appropriate for style changes.

@swissspidy
Copy link
Member

As a plugin author who wants to support multiple WordPress versions, this is a bit cumbersome. I see this new __next36pxDefaultSize prop and use it for my components. Works great in 6.3, but gives deprecation notices in 6.4 (or when using 6.3 with the Gutenberg plugin). I am now forced to have some conditional logic in my code to ensure components look the same across all versions.

Imagine now opting into __next40pxDefaultSize and then with the next release I get deprecation notices all over again despite the code working just fine.

@ciampo
Copy link
Contributor

ciampo commented Oct 24, 2023

Hey all! Together with @mirka , we came up with this strategy (see #46741)

As @tyxla already explained quite well, our main goal was in fact to allow consumers of the library to have a smoother transition as we iteratively tweaked (control) components to all have the same height, which was a requirement coming from the design folks. The alternative would have been to simply tweak the CSS and enforce the new height without a transitional phase, which IMO would have been more disruptive. (cc @WordPress/gutenberg-design )

I personally don't think that this is a breaking change — quite the opposite, in fact.


@peterwilsoncc :

The Dev note says it's a

No, the note never mentions that being a breaking change. It simply lets folks know about an upcoming tweak.

can an HTML class be used instead?

Classnames and DOM structure are not part of a component's APIs and shouldn't be relied upon by consumers of the library.


@swissspidy : I see your point of view and I agree that, if you want to support multiple versions of the plugin, the solution is currently not very elegant. Unfortunately, as we were in the middle of transitioning to the 36px size, a change in design specs forced us to change our strategy to adopt a new height.

But apart from the unexpected 36 to 40px change, I'd argue that the strategy that we came up with allows third-party consumers to have a smoother transition as we update all components — a process that spans across multiple releases.

@peterwilsoncc
Copy link
Contributor Author

No, the note never mentions that being a breaking change. It simply lets folks know about an upcoming tweak.

The dev note says "Sometime after that, the temporary prop will be deprecated and ultimately removed" (my emphasis). It's the introduction to WP Core of an unstable API with the intent to break backward compatibility at a future date.

WordPress Core is intended to be stable and retain backward compatibly. The addition of props with the intent to break them in a future release goes against this.

But apart from the unexpected 36 to 40px change, I'd argue that the strategy that we came up with allows third-party consumers to have a smoother transition as we update all components — a process that spans across multiple releases.

I don't think it's correct to describe this strategy as smooth. __next36... was introduced and discarded, now __next40... is being introduced with the intent to be discarded.

There's no discussion on #46741 about the continued use of the pattern after the previous, 36px, iteration failed.

@ciampo
Copy link
Contributor

ciampo commented Oct 25, 2023

WordPress Core is intended to be stable and retain backward compatibly.

Everyone working on this project and participating to this conversation is very much aware of that, as discussed plenty of times.

with the intent to break backward compatibility at a future date [...] The addition of props with the intent to break them in a future release goes against this.

There was no intent of breaking any compatibility. The prop is designed to be temporary and to be removed without causing any effects (or runtime errors), basically turning into a no-op. There are no compatibility concerns here.

I don't think it's correct to describe this strategy as smooth.

Thank you for your feedback. We will keep this in mind during future UI tweaks, where I guess we'll make similar changes directly to the components without planning for a transitionary phase.

@peterwilsoncc
Copy link
Contributor Author

he prop is designed to be temporary and to be removed without causing any effects (or runtime errors), basically turning into a no-op. There are no compatibility concerns here.

This is the entire point of the ticket!

There are compatibility concerns: the prop is designed to be temporary and to be removed. That's the very definition of an intended backward compatibility break. You can't claim there are no compatibility concerns when you intent to remove a feature.

I would very much like to know how much discussion was had about continuing the pattern of __next*Default after the introduction of the temporary prop __next36pxDefaultSize was introduced but ultimately failed.

@aaronjorbin
Copy link
Member

If someone uses __next36pxDefaultSize or __next40pxDefaultSize and it is later removed, will their code continue to work? If not, that is clearly a backward compatibility break.

@costdev
Copy link
Contributor

costdev commented Oct 26, 2023

Somewhat separate to the backward compatibility queries:

Traditionally WordPress Core's RC phase gives extenders time to test changes and make adjustments; a transitional phase, if you will. Since Gutenberg is in effect early access to part of WordPress Core's functionality, it seems the new styling could be made available in the Gutenberg plugin, and anyone wanting to opt-in to it would install the Gutenberg plugin to get that early access. Can someone help me better understand why the temporary, transitional property is needed at all for merging to wordpress-develop?

@swissspidy
Copy link
Member

swissspidy commented Oct 26, 2023

If someone uses __next36pxDefaultSize or __next40pxDefaultSize and it is later removed, will their code continue to work? If not, that is clearly a backward compatibility break.

I think so. It‘s like a flag for early opt-in. Once it‘s removed, you‘ll get deprecation warnings but the styling should be the same. But you have to keep using the flag & live with the warning if you support multiple WP versions.


@costdev I like that approach!

@ciampo
Copy link
Contributor

ciampo commented Oct 26, 2023

@peterwilsoncc :

That's the very definition of an intended backward compatibility break.

What backward compatibility is exactly being broken here? No features are being removed, no errors (run or build time) are being introduced.

You can't claim there are no compatibility concerns when you intent to remove a feature.

I think there is a bit of a misunderstanding around the subject of this conversation.

There's no feature being discussed here. This is simply a way to coordinate a UI tweak across different components and plugin releases and allow folks to opt-in early into that transition for their own convenience.

Developers either opt-in early, or ignore this completely, and the end result is going to ultimately be the same, once the tweak propagates to all components by default.

@aaronjorbin :

If someone uses __next36pxDefaultSize or __next40pxDefaultSize and it is later removed, will their code continue to work? If not, that is clearly a backward compatibility break.

Yes, things will continue to work as expected after these props are removed. If consumers of these components will continue to pass those props even after removal, things will also continue to work fine.

@swissspidy :

Once it‘s removed, you‘ll get deprecation warnings but the styling should be the same. But you have to keep using the flag & live with the warning if you support multiple WP versions.

That's incorrect. The deprecation warnings are only planned for the final part of the transition. Once the deprecation grace period is over, the props will be removed (ie. ignored) and there won't be any warnings anymore. Passing the prop will simply have no effect.

@costdev:

Can someone help me better understand why the temporary, transitional property is needed at all?

Thank you for sharing your thoughts. As explained above, this temporary property was needed to coordinate a transition that spans across multiple components and multiple releases.


I personally believe that this small change is getting a lot more attention than it should. As explained above, it is not a feature being added and then removed, and there won't be any breakages whatsoever for consumers of the package.

The lesson learned for the future will be to probably apply such tweaks directly to the components without planning for a transitionary phase and assume that the RC release will give folks ample time to make any adjustments.

@swissspidy
Copy link
Member

That's incorrect. The deprecation warnings are only planned for the final part of the transition. Once the deprecation grace period is over, the props will be removed (ie. ignored) and there won't be any warnings anymore. Passing the prop will simply have no effect.

Not sure what to think about that 🤔 Seems like we could then skip the deprecation warning altogether. Less noise.

The lesson learned for the future will be to probably apply such tweaks directly to the components without planning for a transitionary phase and assume that the RC release will give folks ample time to make any adjustments.

In that case, RC is way too late IMHO. Why can't such a change be introduced in a beta already?

@ciampo
Copy link
Contributor

ciampo commented Oct 26, 2023

Not sure what to think about that 🤔 Seems like we could then skip the deprecation warning altogether. Less noise.

👌

In that case, RC is way too late IMHO. Why can't such a change be introduced in a beta already?

It will be part of whatever is the last Gutenberg release to be included in Core, so it will also be in the beta.

@richtabor
Copy link
Member

If someone uses __next36pxDefaultSize or __next40pxDefaultSize and it is later removed, will their code continue to work? If not, that is clearly a backward compatibility break.

Yes, code will continue to work fine.

@richtabor
Copy link
Member

@peterwilsoncc Are there further questions? Or are we good to close this as a non-issue?

@peterwilsoncc
Copy link
Contributor Author

What backward compatibility is exactly being broken here? No features are being removed, no errors (run or build time) are being introduced.

OK, I'll go through it again.

The backward compatibility issue is the introduction of a temporary public API with the intention to deprecate and remove it.

This affects the following use cases:

  • Developers who opted in to __next36pxDefault: their code now throws a deprecation warning and the design now differs between versions of WordPress
  • Developers who opt in to __next40pxDefault: they are being asked to opt in to a property with the promise that eventually their code will throw a deprecation warning before the property is eventually removed
  • Developers who opt in the neither: they are being promised that their decision is temporary and eventually their designs will change regardless
  • Developers using type defections included in wp-scripts: undocumented, when the API is removed will it lead to compilation errors as the types no longer match? This is an issue for developers supporting multiple versions of WP.

To repeat my question form earlier: I would very much like to know how much discussion was had about continuing the pattern of __next*Default after the introduction of the temporary prop __next36pxDefaultSize was introduced but ultimately failed.


@richtabor My main question is the last paragraph above.

The larger question is why the __intentedToBeRemoved is still being used in WordPress Core (ie, not the plugin) after all the work went in to the introduction of the private API packages but that's can be dealt with elsewhere.

@ciampo
Copy link
Contributor

ciampo commented Oct 27, 2023

Developers who opted in to __next36pxDefault their code now throws a deprecation warning and the design now differs between versions of WordPress

The deprecation warning is in place precisely to inform about that. The change in design is the point of the change, so that's not a negative in my opinion.

Developers who opt in to __next40pxDefault: they are being asked to opt in to a property with the promise that eventually their code will throw a deprecation warning before the property is eventually removed

That's incorrect. Quoting from #46741, "add a deprecated() for any usages that don't have the __next40pxDefaultSize prop enabled.". That warning was planned for folks who haven't opted into the prop yet.

Also, as discussed in a message above, I'm happy not to introduce a warning if that's what consumers of the library prefer.

Developers who opt in the neither: they are being promised that their decision is temporary and eventually their designs will change regardless

It's really more like they are being made aware with ample notice about a design tweak (fix) that is being applied across the editor.

Developers using type defections included in wp-scripts: undocumented, when the API is removed will it lead to compilation errors as the types no longer match? This is an issue for developers supporting multiple versions of WP.

Developers using type defs may experience a build time error if the prop is removed from the type defs and they are still passing that prop to the component. At the same time, I assume that developers supporting multiple versions are either able to load the correct type defs as needed, or otherwise are already dealing with similar type errors when updating version. It is simply a byproduct of of type linting works, and to be honest a great way to flag any potential mismatch clearly, instead of causing runtime inconsistencies at a later point.

For example, if a new prop is added and a developer needs to support an older version where that prop wasn't defined, wouldn't that generate the same potential build errors?

I would very much like to know how much discussion was had about continuing the pattern of __next*Default after the introduction of the temporary prop __next36pxDefaultSize was introduced but ultimately failed.

I personally don't see the failure — the design specs changed, and we tweaked our API strategy to follow that. The strategy has been working as designed and has allowed us to continue to update the editor UI as needed.

The larger question is why the __intentedToBeRemoved is still being used in WordPress Core (ie, not the plugin) after all the work went in to the introduction of the private API packages but that's can be dealt with elsewhere.

First of all, I just wanted to note that this API strategy was decided at a time when private APIs were not yet introduced.

As mentioned before, the lesson learned for the future will be to probably apply such tweaks directly to the components without planning for a public API to help third-party developers through a transitionary phase, assuming that beta/RC releases will be enough time for folks to make any necessary tweaks.

Are there further questions? Or are we good to close this as a non-issue?

I personally don't have anything else to add to this conversation.

@peterwilsoncc
Copy link
Contributor Author

#46741 was logged in 2022, after the introduction of the private APIs package.

Let's consider @swissspidy's situation above of trying to properly support multiple versions of WordPress

  • Try to control for the change by following the recommended action and using __next36pxDefault. In WordPress 6.4 doing the right thing recommended by the editor team will throw a notice, leaving the options
    • Remove __next36pxDefault, re-test against older versions of WP to make sure nothing is broken
    • Keep __next36pxDefault and get deprecation notices as a result of doing the right thing. Close support tickets reporting the deprecation as wontfix
  • For WordPress 6.4: Opt in to __next40pxDefault
  • For WordPress x.x when all internal components have migrated: avoid deprecation notices. A good thing
  • For WordPress x.x when the property is deprecated: it's undocumented but I presume at this point deprecation notices will be thrown warning of the impeding removal. Options
    • Remove __next40pxDefault, re-test against older versions of WP to make sure nothing is broken
    • Keep __next40pxDefault and get deprecation notices as a result of doing the right thing. Close support tickets

Each time a temporary prop that is designed to be removed is introduced, you place a burden on plugin developers. In this case you're introducing that burden for plugin developers trying to do the right thing and follow recommended practices. Plugin devs can not win.

Changes to these APIs require plugin developers go to some length to avoid deprecation notices. Unplanned changes are sometimes necessary and can't be helped but to plan to add this burden is poor form. It's a very old example (republished just now to avoid identifying information) but this is an example of lengths plugin devs need to go to to deal with these changes.

As mentioned before, the lesson learned for the future will be to probably apply such tweaks directly to the components without planning for a public API to help third-party developers through a transitionary phase, assuming that beta/RC releases will be enough time for folks to make any necessary tweaks.

This isn't a great approach either. I think actually committing to maintaining backward compatibility once code is released in WordPress Core is the correct approach.

I personally don't have anything else to add to this conversation.

It's up to y'all whether you close this as unplanned or not. It's clear the concerns are being dismissed and the intention is to ship yet another temporary property, ask plugin developers to opt-in to it, only for it to be removed later.

It's a poor choice, and I am getting messages from experienced contributors thanking me for raising this topic again. However, it's clear the move fast and break things philosophy that is helpful in the plugin is once again being adopted for WordPress releases in which it is not helpful.

@richtabor
Copy link
Member

richtabor commented Oct 31, 2023

ask plugin developers to opt-in to it, only for it to be removed later.

"Removed" in the sense that this prop becomes the default, not removed as in breaking/unintended changes. I'm not following the concern? You can opt to not support the __next40pxDefault if you'd like and you'll inherit the changes when it's the current default.

@peterwilsoncc
Copy link
Contributor Author

As those of us on this ticket have spent two weeks discussing whether adding a property with the intent to remove it later constitutes a promised break instead of finding a solution, we've reached the point that it's too late to resolve this for WordPress 6.4.

Once again the situation arises in which WordPress Developer experience is being prioritized over the far more common user, theme and plugin developer experience.

As the relevant dev-note includes an implicit request developers use this temporary property, this is the path forward:

  • the use of __next40pxDefaultSize must never throw a deprecation notice. Doing so will put theme and plugin developers in the situation of not opting in throwing a deprecation notice while opting in also throws a deprecation notice.
  • __next40pxDefaultSize needs to be included in the definitions once the functionality is removed (possibly commented out) to avoid future naming collisions. A comment will need to explain why it is there.
  • __next40pxDefaultSize can never be removed from the typescript definitions so developers making use of them don't end up with compilation failures.

As the __next36pxDefaultSize temporary property was introduced in WordPress 6.1 and never fully implemented before being replaced with a new iteration, the pattern is a failure and needs to be avoided in the future. Developers opting in to the former property in an attempt to experience a smoother transition are having a rougher transition that developers who did not.

Copy link

github-actions bot commented Dec 2, 2023

Hi,
This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps.
Thanks for helping out.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Dec 2, 2023
@mirka
Copy link
Member

mirka commented Jan 24, 2024

Once again the situation arises in which WordPress Developer experience is being prioritized over the far more common user, theme and plugin developer experience.

Quite the opposite — if we had prioritized the core dev experience, we would've shipped the design changes without a transitional prop. We put in a lot of extra effort to plan and implement this transitional pathway specifically for theme and plugin devs, so there is no sudden disruption to their UIs.

There are pros/cons to every approach. The deprecation warnings (as outlined in #46741) and TypeScript compilation failures are intended to be informational, but if we think those are more harmful than helpful, we can most certainly accommodate that. Can we assume that is a pretty universal sentiment? If so, we can drop the deprecation warnings and just publish a Dev Note. We can keep the props in TS and just mark them as @deprecated so it still compiles.

As the __next36pxDefaultSize temporary property was introduced in WordPress 6.1 and never fully implemented before being replaced with a new iteration, the pattern is a failure and needs to be avoided in the future. Developers opting in to the former property in an attempt to experience a smoother transition are having a rougher transition that developers who did not.

I empathize with that, but that is a discussion about how we can avoid unforeseen changes in design/product direction, rather than a failure of a transition pattern. In fact, the possibility of an unforeseen design direction change (such as what actually happened) was specifically taken into account in planning the transition pattern.

So while it's incredibly useful to get feedback like this from extenders so we can better balance the pros/cons of a transition strategy, it's often impossible to make the process completely frictionless for every consumer. And that includes keeping all APIs around forever, which accumulates cost in bundle size and API clarity.

@mirka
Copy link
Member

mirka commented Jan 24, 2024

Based on the suggestions from @peterwilsoncc, would these two action items (as amendments to the size transition strategy in #46741) be sufficient to close this issue?

  • Drop all deprecation warnings (existing and planned), and rather rely on just a Dev Note to inform consumers.
  • Keep both __next36pxDefaultSize and __next40pxDefaultSize in the TypeScript definitions (only mark them as @deprecated) so they compile indefinitely. This will also prevent future naming collisions.

@peterwilsoncc
Copy link
Contributor Author

@mirka This sounds good.

I also think a discussion would be helpful to figure out a way to ease these design transitions without using temporary __ properties at the root level would be helpful. Whether that be designThings< Array > or designIteration< number > can be figured out in the discussion along with some good names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Testing Needs further testing to be confirmed. [Package] Components /packages/components [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended
Projects
Status: Done 🎉
Development

Successfully merging a pull request may close this issue.

9 participants