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

Editor: Deprecate usage of RichText provider component #9465

Merged
merged 2 commits into from Aug 31, 2018

Conversation

@gziolo
Member

gziolo commented Aug 30, 2018

Description

This PR marks RichTextProvider component as deprecated since it is no longer needed to use undo/redo features.

I discovered it when trying to fix rarely failing undo test:
#5685 (comment)

Open question

Should we keep RichTextProvider active as it is at the moment, or remove it from the list of the providers injected into the app. Technically, it's no longer necessary unless some plugins depend on it.

How has this been tested?

npm run test-e2e

End to end test should still pass.

Types of changes

Refactoring with the deprecation warning.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added this to the 3.8 milestone Aug 30, 2018

@gziolo gziolo self-assigned this Aug 30, 2018

@gziolo gziolo added this to In Progress in API freeze via automation Aug 30, 2018

@gziolo gziolo requested review from youknowriad and iseulde Aug 30, 2018

@gziolo gziolo added the Code Quality label Aug 30, 2018

@gziolo gziolo requested a review from WordPress/gutenberg-core Aug 30, 2018

@aduth aduth self-requested a review Aug 30, 2018

@aduth

aduth approved these changes Aug 30, 2018

Nice. I saw this the other day and had a similar thought that it should no longer be necessary with RichText being in editor.

Should we keep RichTextProvider active as it is at the moment, or remove it from the list of the providers injected into the app. Technically, it's no longer necessary unless some plugins depend on it.

I think it's not harming anything to keep around for the duration of the deprecation.

@@ -1,5 +1,9 @@
Gutenberg's deprecation policy is intended to support backwards-compatibility for two minor releases, when possible. The current deprecations are listed below and are grouped by _the version at which they will be removed completely_. If your plugin depends on these behaviors, you must update to the recommended alternative before the noted version.
## 4.0.0

This comment has been minimized.

@aduth

aduth Aug 30, 2018

Member

Should we start including the deprecation note in the relevant package's CHANGELOG.md as well? I have been and am inclined to think we should.

@aduth

aduth Aug 30, 2018

Member

Should we start including the deprecation note in the relevant package's CHANGELOG.md as well? I have been and am inclined to think we should.

This comment has been minimized.

@youknowriad

youknowriad Aug 30, 2018

Contributor

While I agree it would be great, I'm afraid we're adding more on more stuff to think about for the reviewer/merger.

@youknowriad

youknowriad Aug 30, 2018

Contributor

While I agree it would be great, I'm afraid we're adding more on more stuff to think about for the reviewer/merger.

This comment has been minimized.

@aduth

aduth Aug 30, 2018

Member

But you agreed to it yesterday 😛

https://wordpress.slack.com/archives/C5UNMSU4R/p1535567253000100

Honestly, if we wanted to limit where we note, I'd say CHANGELOG would be the more sensible default. A document like this one could theoretically be generated by scanning each CHANGELOG for its deprecation sections.

@aduth

aduth Aug 30, 2018

Member

But you agreed to it yesterday 😛

https://wordpress.slack.com/archives/C5UNMSU4R/p1535567253000100

Honestly, if we wanted to limit where we note, I'd say CHANGELOG would be the more sensible default. A document like this one could theoretically be generated by scanning each CHANGELOG for its deprecation sections.

This comment has been minimized.

@youknowriad

youknowriad Aug 30, 2018

Contributor

I "accidentally" agreed to it, I never thought about deprecations :P

The reason it's not easy to automate is the version where it will be removed (one mentions the Gutenberg version, the other the npm package version).

@youknowriad

youknowriad Aug 30, 2018

Contributor

I "accidentally" agreed to it, I never thought about deprecations :P

The reason it's not easy to automate is the version where it will be removed (one mentions the Gutenberg version, the other the npm package version).

This comment has been minimized.

@aduth

aduth Aug 30, 2018

Member

(one mentions the Gutenberg version, the other the npm package version).

It's good to raise this actually, since we include the Gutenberg version in the deprecated function call despite the Gutenberg plugin not having any meaning of its own in the context of the package alone.

@aduth

aduth Aug 30, 2018

Member

(one mentions the Gutenberg version, the other the npm package version).

It's good to raise this actually, since we include the Gutenberg version in the deprecated function call despite the Gutenberg plugin not having any meaning of its own in the context of the package alone.

This comment has been minimized.

@gziolo

gziolo Aug 31, 2018

Member

There is a very narrow group of people who introduce deprecations so we might have more strict rules which will benefit those who consume those packages. I'm fine with adding a section in the changelog like this:

### Deprecation

- `wp.editor.RichTextProvider` flaged for deprecation. Please use `wp.data.select( 'core/editor' )` methods instead.

In this case, it will be a patch release so we probably should include it anyway.

@gziolo

gziolo Aug 31, 2018

Member

There is a very narrow group of people who introduce deprecations so we might have more strict rules which will benefit those who consume those packages. I'm fine with adding a section in the changelog like this:

### Deprecation

- `wp.editor.RichTextProvider` flaged for deprecation. Please use `wp.data.select( 'core/editor' )` methods instead.

In this case, it will be a patch release so we probably should include it anyway.

This comment has been minimized.

@aduth

aduth Aug 31, 2018

Member

In this case, it will be a patch release so we probably should include it anyway.

For a deprecation, we should make it a minor release:

Minor version [...] MUST be incremented if any public API functionality is marked as deprecated.

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

@aduth

aduth Aug 31, 2018

Member

In this case, it will be a patch release so we probably should include it anyway.

For a deprecation, we should make it a minor release:

Minor version [...] MUST be incremented if any public API functionality is marked as deprecated.

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

This comment has been minimized.

@gziolo

gziolo Aug 31, 2018

Member

Yeah, good catch. I had some blurry memories it should be minor but I couldn't find a reason why. Let's follow spec 👍

@gziolo

gziolo Aug 31, 2018

Member

Yeah, good catch. I had some blurry memories it should be minor but I couldn't find a reason why. Let's follow spec 👍

This comment has been minimized.

@aduth

aduth Aug 31, 2018

Member

It's good to raise this actually, since we include the Gutenberg version in the deprecated function call despite the Gutenberg plugin not having any meaning of its own in the context of the package alone.

Noting that I added this as an agenda item for next week's core JS meeting.

https://docs.google.com/document/d/1NYrgxr1dkeoEa12bHB9rr4l1UhZJIUPAzvEeFBlfYvs/edit

@aduth

aduth Aug 31, 2018

Member

It's good to raise this actually, since we include the Gutenberg version in the deprecated function call despite the Gutenberg plugin not having any meaning of its own in the context of the package alone.

Noting that I added this as an agenda item for next week's core JS meeting.

https://docs.google.com/document/d/1NYrgxr1dkeoEa12bHB9rr4l1UhZJIUPAzvEeFBlfYvs/edit

This comment has been minimized.

@gziolo

gziolo Aug 31, 2018

Member

Addressed with 0613c4b, there are breaking changes so it's going to be major anyway :)

By the way, I really like how useful this changelog becomes.

@gziolo

gziolo Aug 31, 2018

Member

Addressed with 0613c4b, there are breaking changes so it's going to be major anyway :)

By the way, I really like how useful this changelog becomes.

@gziolo gziolo merged commit 6a5ac6a into master Aug 31, 2018

2 checks passed

codecov/project 50.23% (-0.01%) compared to 9f2b3e3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Aug 31, 2018

@gziolo gziolo deleted the update/undo-provider branch Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment