-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Comments
__next40pxDefaultSize
promises BC break in WordPress Core.__next40pxDefaultSize
promises BC break in WordPress Core.
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? |
The Dev note says it's a
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 |
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? |
@tyxla If something like this must be done, can an HTML class be used instead? It's more appropriate for style changes. |
As a plugin author who wants to support multiple WordPress versions, this is a bit cumbersome. I see this new Imagine now opting into |
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.
No, the note never mentions that being a breaking change. It simply lets folks know about an upcoming tweak.
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 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. |
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.
I don't think it's correct to describe this strategy as smooth. There's no discussion on #46741 about the continued use of the pattern after the previous, 36px, iteration failed. |
Everyone working on this project and participating to this conversation is very much aware of that, as discussed plenty of times.
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.
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. |
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 |
If someone uses |
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 |
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! |
What backward compatibility is exactly being broken here? No features are being removed, no errors (run or build time) are being introduced.
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.
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.
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.
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. |
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. |
Yes, code will continue to work fine. |
@peterwilsoncc Are there further questions? Or are we good to close this as a non-issue? |
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:
To repeat my question form earlier: I would very much like to know how much discussion was had about continuing the pattern of @richtabor My main question is the last paragraph above. The larger question is why the |
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.
That's incorrect. Quoting from #46741, "add a Also, as discussed in a message above, I'm happy not to introduce a warning if that's what consumers of the library prefer.
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 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 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.
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.
I personally don't have anything else to add to this conversation. |
#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
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.
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.
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. |
"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 |
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:
As the |
Hi, |
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
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. |
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?
|
@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 |
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
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
The text was updated successfully, but these errors were encountered: