Deprecate interface package's preference APIs #39418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Task
|
Size Change: +2.77 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
talldan
commented
Mar 14, 2022
| * preferences to migrate to the interface | ||
| * package. | ||
| */ | ||
| export function migrateFeaturePreferencesToInterfaceStore( |
There was a problem hiding this comment.
This function was unused, so I removed it.
talldan
commented
Mar 14, 2022
| @@ -384,13 +384,13 @@ describe( 'persistence', () => { | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'migrateFeaturePreferencesToInterfaceStore', () => { | |||
There was a problem hiding this comment.
To most easily review this, toggle on 'Hide whitespace' in the little cog menu at the top.
Otherwise the diff looks worse than it is. Lots of the other tests were all incorrectly nested in this describe, and all I did was:
- un-nest incorrectly nested tests
- delete the tests for the removed function
- add the tests for the new function.
talldan
commented
Mar 14, 2022
| import { isFeatureActive } from '../selectors'; | ||
|
|
||
| describe( 'selectors', () => { | ||
| describe( 'isFeatureActive', () => { |
There was a problem hiding this comment.
I've deleted this as the selector is tested extensively in the actions.js test file.
talldan
force-pushed
the
deprecate/interface-preference-apis
branch
from
1ef3e81 to
351cc81
Compare
18 tasks
|
I realised there's a bug in I've put together a fix in 0dba039. I'm including it in this PR, as otherwise the conflicts will be difficult to handle. I think it'd be best to cherry-pick this PR into the Gutenberg 12.8 release, both so that the interface APIs continue to work in that release, and also so that local storage data isn't accidentally removed. |
andrewserong
approved these changes
Mar 16, 2022
There was a problem hiding this comment.
The changes look good to me @talldan! Thanks for the explanations in the PR description and extra comments, it very much helps to wrap my head around the changes 😀 — it's testing well according to the testing steps 👍
The only feedback I noted was a few comments where the existing code uses optional chaining to set a value, but then attempts to spread the value using ... — I suspect that there've never been any errors here because the values are always an object, rather than undefined, and I couldn't break it during testing 🤔
Feel free to ignore those comments if they're not applicable here. If you think they should be addressed I suppose we could change them to ...( myValue || {} ) and / or add tests for calling those functions with a state that doesn't have a preferences key?
| continue; | ||
| } | ||
|
|
||
| // Skip this scope if there are no features to migrates |
There was a problem hiding this comment.
Tiny typo 😁
Suggested change
| // Skip this scope if there are no features to migrates | |
| // Skip this scope if there are no features to migrate. |
|
This is working as described for me. I can confirm that core preferences aren't being migrated as well. Not entirely sure if relevant, but for the heck of it I installed a third party plugin to check that all is well with settings preferences as well.
Tests pass ✅ |
|
Thanks for the reviews. 🎉 |
priethor
pushed a commit
that referenced
this pull request
Mar 16, 2022
* Deprecate and proxy existing feature preference API in interface package * Remove peristence of preferences reducer * Migrate third party preferences from the interface package * Improve deprecation message * Ensure non-preference data is retained after any migration
youknowriad
reviewed
Mar 22, 2022
| dispatch.setFeatureValue( scope, featureName, ! currentValue ); | ||
| return function ( { registry } ) { | ||
| deprecated( `wp.dispatch( 'core/interface' ).toggleFeature`, { | ||
| version: '6.0', |
There was a problem hiding this comment.
I'm not mistaken, I think you meant to use since (version of deprecation) instead of version (version of removal) here, right? #39640
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #31965
What?
Deprecates the feature preference actions and selectors in the interface store and makes them select and dispatch from the new preferences package.
Why?
In #38873 a new package was introduced which replaces what these selectors and actions do.
The editors that were previously using the interface package for feature preferences have already been migrated to use the new preferences package (#39084, #39112, #39115).
These old APIs now need to be deprecated, but they need to also still work for any third party usage.
How?
Testing Instructions
There's no user facing way to test this, the easiest option is to use the API in the browser console the way a plugin would.
trunkand build