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

Add a backward compatibility document #18499

Merged
merged 33 commits into from
Nov 14, 2019
Merged

Conversation

youknowriad
Copy link
Contributor

closes #4738

Among team members and WordPress developers, there's a lot of assumptions on backward compatibility and knowledge that is not shared properly to all contributors.

While this first document is probably unperfect, it is at least a good starting point to understand the backward compatibility policy in Gutenberg and WordPress.

@youknowriad youknowriad self-assigned this Nov 14, 2019
@youknowriad youknowriad added the [Type] Developer Documentation Documentation for developers label Nov 14, 2019
@youknowriad youknowriad requested a review from a team November 14, 2019 09:53

Gutenberg follows the WordPress backwards compatibility principles. This means Backward compatibility for producation public API is guaranteed. In some rare occasions breaking backward compatibility is unavoidable, in that case, the breakage should be as small as possible and the breakage should be documented as clearly as possible to third-party developpers using devnotes.

## What qualifies as a production public API:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "production-ready"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non just production. Some APIs are not production APIs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, should it be named production API then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean remove "public"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no :D I meant: if these are not just "production APIs", why call them a "production public API"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 hmm... ya so I guess what you're distinguishing between here is packages built for production use vs packages built for development use and both have public apis but we only care about backwards-compatibility when it comes to packages built for production?

If so, then maybe the heading could be:

What qualifies as a public api for production use?

Again though, this is a nit. It's completely fine with me if you just want to resolve and use the text as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards keeping it as is because it's the same term used in the previous sentence. We can iterate of course.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully get the term. You're saying that "production public API" consists of "production packages" and "development packages". This is a bit confusing to me. So the "development packages" is also "production public API"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

production public API is just public APIs exported from production packages

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The confusion comes from the fact that we never address the question:

"What qualifies as a production public API?".

Somewhere after defining the two types of packages we have to write:

"A production public API is a public API from a production package."

youknowriad and others added 3 commits November 14, 2019 11:24
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
youknowriad and others added 7 commits November 14, 2019 11:26
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I ❤️ this! I'm a huge proponent for documenting things like this as it provides a reference to point to when the question comes up. So 👍 👍

I do have some concerns that we're setting up false expectations by using the term guarantee though. While I appreciate it grants additional significance to how backwards-compatibility is viewed, WP itself doesn't explicitly guarantee backwards compatibility anywhere (that I know of). So my suggestions are mostly around softening the language a bit.


Gutenberg follows the WordPress backwards compatibility principles. This means Backward compatibility for producation public API is guaranteed. In some rare occasions breaking backward compatibility is unavoidable, in that case, the breakage should be as small as possible and the breakage should be documented as clearly as possible to third-party developpers using devnotes.

## What qualifies as a production public API:
Copy link
Contributor

@nerrad nerrad Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would read better to me if this was phrased as "production ready public api"

* should be constrained as much as possible to a small surface area of the API.
* should be documented as clearly as possible to third-party developers using dev notes.

## What qualifies as a production public API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## What qualifies as a production public API
## What qualifies as a production public API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do that, we should do it for all the titles that are written as questions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Hows" read more like statements.

Although technically, this can also be a statement, it reads more like a question, but it might be just me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a content clause, it's perfectly fine as is.

youknowriad and others added 16 commits November 14, 2019 17:55
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
@youknowriad youknowriad merged commit dccd21c into master Nov 14, 2019
@youknowriad youknowriad deleted the add/backward-compatibility-doc branch November 14, 2019 17:16
@gziolo
Copy link
Member

gziolo commented Nov 15, 2019

Awesome work on this document. I’m really happy to see all that clarified and now easy to share during the code review.

I’m happy about the part which states that class names aren’t part of the public API. I assumed they are based on the fact that theme authors use them extensively.

@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019

## Class names and DOM updates

Class names and DOM nodes used inside the tree of React components are not considered part of the public API and can be modified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking today how unsustainable and regrettable I find the changes of #14420. I'd be happy to see those old classes removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we have just started the development cycle for WordPress 5.4 it should be a good moment to clean this up. We would need to include it in dev notes, but given that it's a simple string replace action for theme authors, it should be straightforward to apply for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible to remove them today, especially as we refactor some of these components. We just need to make sure to write dev notes about these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a note to put together a pull request. I think it will save much confusion for everyone, since the duplication of class names makes it very unclear which is the preferred, and how to name new classes. it seems reasonable enough to me that a DevNote should cover this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up at #19050


## Class names and DOM updates

Class names and DOM nodes used inside the tree of React components are not considered part of the public API and can be modified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One fundamental concern I have here (and apologies for the late revisit) is that currently it is only possible to build a block which renders child blocks horizontally by targeting class names, as the InnerBlocks component does not offer declarative UI configuration (e.g. isHorizontal or shouldWrapBlocks-style props).

I love the idea here but it feels like this is a case where it might be warranted to make an exception until a declarative alternative is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, I think the Navigation block has started building a declarative API for that, the columns block is also a horizontal container, it should be possible to offer a better API here.

Copy link
Member

@aduth aduth Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this example contradicts the general recommendation though. If there's need for something like this, we shouldn't want that the developer feel need to target the classes, but instead offer it as an option via the component interface. It appears that's what's already being said in this comment chain, but I want to point out that I think it's consistent with treating the class names as unstable.

Put another way: I don't think there should be exceptions, because these aren't supported use-cases. If they were supported use-cases, there would be a component interface for them, and not a reliance on a specific class name. There's nothing to stop a developer from choosing to target the class names anyways if they so choose, but the framework shouldn't be held responsible to upholding support for that usage.

For me, if the purpose of this document is to outline the intended support commitments, then we should be consistent to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a statement to docs regarding backwards compatibility
8 participants