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

Docs: Clarify "Experimental and Unstable APIs" guidelines #14557

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 21, 2019

This pull request seeks to clarify the wording of the Coding Guidelines "Experimental and Unstable APIs" section, toward the goals of:

  • Providing clearer messaging to potential external consumers that there is no support commitment
  • Better contrasting the specific differences between what constitutes an API as being "experimental" vs. "unstable"
  • Noting that an experimental API must inherently be part of a breaking change as part of its stabilization (by the fact that __experimental is to be removed from its exposed name)
  • Emphasizing that these APIs should be short-lived

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.

In general I like these proposed changes (hence the approval). How does this impact the automatic docs generation? I think I've noticed a few cases where auto-docs generation has picked up prefixed functions and thus would violate these guidelines. It's possible that's already been rectified somewhere but just thought I'd bring it up.

@aduth
Copy link
Member Author

aduth commented Mar 21, 2019

How does this impact the automatic docs generation?

By default, it's meant to ignore anything containing these keywords:

'--ignore "unstable|experimental"',

I think I've noticed a few cases where auto-docs generation has picked up prefixed functions and thus would violate these guidelines. It's possible that's already been rectified somewhere but just thought I'd bring it up.

I'll defer to @nosolosw on this.


```js
export { __unstableDoAction } from './api';
export { __experimentalDoExcitingExperimentalAction } from './api';
export { __unstableDoTerribleAwfulAction } from './api';
Copy link
Member

Choose a reason for hiding this comment

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

Love this example ❤️

Related, is there any way of telling ESLint that __exeperminental* and __unstable* prefixes are fine? It yells when you try to use. It is fine but a little bit annoying to use such method. At least for experimental, it could be less strict.

Copy link
Member Author

Choose a reason for hiding this comment

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

Related, is there any way of telling ESLint that __exeperminental* and __unstable* prefixes are fine? It yells when you try to use. It is fine but a little bit annoying to use such method. At least for experimental, it could be less strict.

Could you share an example? I think I've seen it before where we use the non-conventional unstable__Whatever, but with the correct, leading underscores, it doesn't trip ESLint's camelcase rule:

https://eslint.org/demo/#eyJ0ZXh0IjoiY29uc3QgeyBfX2V4cGVyaW1lbnRhbEZvbyB9ID0gd2luZG93O1xuY29uc3QgeyBleHBlcmltZW50YWxfX0ZvbyB9ID0gd2luZG93OyIsIm9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6OSwic291cmNlVHlwZSI6InNjcmlwdCIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6eyJjYW1lbGNhc2UiOjJ9LCJlbnYiOnt9fX0=

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/WordPress/gutenberg/pull/14551/files#diff-33e0ac9c44b16a617039a563d4c065f3R60

It looks like I followed the wrong usage (unstable__bootstrapServerSideBlockDefinitions) in the first place 🤣

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I like all clarifications included in this PR.

I was wondering whether we should replace unstable keyword with something scarier:

export { __WILL_BE_REMOVED_doTerribleAwfulAction } from './api';

I'm not sure about the correct phrasing but sharing how it could be visually explained to the observers who won't read docs but will feel a strong desire to use such API method :)

@oandregal
Copy link
Member

oandregal commented Mar 22, 2019

I think I've noticed a few cases where auto-docs generation has picked up prefixed functions and thus would violate these guidelines. It's possible that's already been rectified somewhere but just thought I'd bring it up.

Just checked for unstable/experimental in Markdown files, and this is what I've found.

experimental:

  • docs/designers-developers/developers/data/data-core-annotations.md
  • docs/designers-developers/developers/data/data-core-editor.md

unstable:

  • docs/designers-developers/developers/data/data-core-block-editor.md
  • docs/designers-developers/developers/data/data-core-editor.md
  • packages/block-editor/README.md (UnstableRichTextInputEvent)

Of those, only block-editor has set up auto-generated docs at the moment. I'll get to fix that export (edit: see #14565).

Please, if you see any export that shouldn't be auto-documented, ping me and I'll get to fix it! :)

@aduth
Copy link
Member Author

aduth commented Mar 25, 2019

I was wondering whether we should replace unstable keyword with something scarier:

There was a previous Core JS agenda item on this exact point:

There was a conversation recently on Github where it became evident that marking an API as unstable is not sufficient enough in its own right to scare people away from using them (and subsequently being frustrated that they’re not documented).

https://make.wordpress.org/core/2019/01/29/javascript-chat-summary-january-29-2019/

In the meeting, it seemed generally agreed upon that there was not a need for changing the name.

I'm happy to revisit it, but also think that (a) changing conventions is painful, (b) there's no perfect solution and the current convention is a reasonable middle-ground and (c) with new encouragement as of this pull request, unstable APIs should be very short-lived and not commonly encountered.

@gziolo
Copy link
Member

gziolo commented Mar 26, 2019

I'm happy to revisit it, but also think that (a) changing conventions is painful, (b) there's no perfect solution and the current convention is a reasonable middle-ground and (c) with new encouragement as of this pull request, unstable APIs should be very short-lived and not commonly encountered.

I totally get your point and respect the previous decision made. It's fine to proceed as is and that's why I added my ✅ Ideally we don't have to use these prefixes at all rather than spend too much time thinking how to name them 😅

@gziolo gziolo merged commit 7ec933b into master Mar 26, 2019
@gziolo gziolo deleted the update/coding-guidelines-unstable-apis branch March 26, 2019 07:06
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
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.

5 participants