Migrate site editor to use new preferences package #39158
Merged
Conversation
This file contains hidden or 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] Enhancement
Contributor
Author
|
Near the end of my day and I haven't tested this yet, and also tests are likely to fail, so keeping this a draft until I can finish it off tomorrow. |
18 tasks
|
Size Change: +3.11 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
talldan
changed the title
This was referenced Mar 3, 2022
talldan
force-pushed
the
migrate/edit-site-to-use-new-preferences
branch
from
3e23219 to
6088be3
Compare
talldan
commented
Mar 3, 2022
Comment on lines
+36
to
+48
| /** | ||
| * Helper for getting a preference from the preferences store. | ||
| * | ||
| * This is only present so that `getSettings` doesn't need to be made a | ||
| * registry selector. | ||
| * | ||
| * It's unstable because the selector needs to be exported and so part of the | ||
| * public API to work. | ||
| */ | ||
| export const __unstableGetPreference = createRegistrySelector( | ||
| ( select ) => ( state, name ) => | ||
| select( preferencesStore ).get( 'core/edit-site', name ) | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
This is the part I'm most unhappy with. Ideally any helper is internal only, but the registry selector needs to be exported to work.
The separate selector is also needed for the memoized getSettings selector.
If there's a different way to do this that anyone knows of, that'd be great.
Member
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
I think we may just have to inline this function so that
isFeatureActive,getSettingsare registry selectors that callselect( preferencesStore ).getdirectly.
If you insist on having a getPreference for code re-use (I'd argue it's maybe not worth it since it's only one line) then getPreference could accept select as an argument.
const getPreference = ( select, name ) =>
select( preferencesStore ).get( 'core/edit-site', name );
export const isFeatureActive = createRegistrySelector(
( select ) => ( state, featureName ) => {
return !! getPreference( select, featureName );
}
);
Contributor
Author
There was a problem hiding this comment.
The thing that makes it difficult is the second argument to createSelector (which is used for memoizing getSettings) is one of the places __unstableGetPreference needs to be used. That has no way to access select from what I can see:
gutenberg/packages/edit-site/src/store/selectors.js
Lines 105 to 151 in c366a96
I suppose it would be possible to import select from the data store for this purpose. I don't know whether that's an ok practice.
Member
There was a problem hiding this comment.
I'm afraid there are only two options available:
- export the
__unstableGetPreferenceselector as part of the public API. The only reason we need to do that is that the code that initializes the public selectors assigns to theselector.registryfield. - wrap all the selectors that use
__unstableGetPreferencewithcreateRegistrySelectorand inline the helper.
import
selectfrom the data store for this purpose
That would mean that the store can be ever registered only with the default registry and never any other one. Because such an import would hardcode a reference to the default registry to the selector.
createRegistrySelector will already fail when you try to register its store with two registries -- because they will fight over the selector.registry field, both assigning to them. We'll need to rethink the entire concept at some point.
Member
There was a problem hiding this comment.
The thing that makes it difficult is the second argument to
createSelector
Ah nuts 🥜
We'll need to rethink the entire concept at some point.
Yeah. I wonder if we can do something similar to thunks where selectors (and the second argument of createSelector) can return a function which is passed an object containing registry, select, etc.
Contributor
Author
There was a problem hiding this comment.
That would mean that the store can be ever registered only with the default registry and never any other one. Because such an import would hardcode a reference to the default registry to the selector.
I see. I remember there being a reason for avoiding this practice, but couldn't remember what it was.
Seems like making it __unstable is probably the best trade-off for now. Thanks for all the feedback.
talldan
force-pushed
the
migrate/edit-site-to-use-new-preferences
branch
from
eeb7311 to
14a27e9
Compare
talldan
force-pushed
the
migrate/edit-site-to-use-new-preferences
branch
from
14a27e9 to
c366a96
Compare
noisysocks
approved these changes
Mar 8, 2022
Member
noisysocks
left a comment
noisysocks
left a comment
There was a problem hiding this comment.
Works a treat 👍 and look at all that red! 📉
| import { store as editSiteStore } from '../../../store'; | ||
| import { store as preferencesStore } from '@wordpress/preferences'; | ||
|
|
||
| export default function WelcomeGuideMenuItem() { |
Member
There was a problem hiding this comment.
Why can't we use a PreferenceToggleMenuItem instead of WelcomeGuideMenuItem? Because we don't want to show a tick ✔️?
Contributor
Author
There was a problem hiding this comment.
Yep, it would also have the wrong aria role (menuitemcheckbox vs plain menuitem)
| reducer, | ||
| actions, | ||
| selectors, | ||
| persist: [ 'preferences' ], |
| } | ||
|
|
||
| describe( 'actions', () => { | ||
| describe( 'toggleFeature', () => { |
Member
There was a problem hiding this comment.
Maybe it's worth keeping a unit test for toggleFeature around? I'm not sure. On the one hand, it would be a very dumb test that barely tests anything. On the other hand, we can't ever delete toggleFeature and so it's all that we would have to ensure that there aren't any typos inside toggleFeature.
Contributor
Author
There was a problem hiding this comment.
I think I was just happy to delete stuff. 😄
I'll add a commit that brings it back.
Contributor
Author
There was a problem hiding this comment.
Done in 94eef52.
I've only added back the action test as its easier to test a registry selector there, but I've expanded the test so that it does more testing of the selector.
| select: jest.fn( () => ( { getEntityRecords } ) ), | ||
| }; | ||
|
|
||
| describe( 'isFeatureActive', () => { |
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.
Description
Addresses some of #31965
Migrates the site editor to use the new preferences package (introduced in #38873). Previously this package had its own preferences implementation, but now it uses a common package, which should speed up the process of implementing new preferences.
The main goal here is that nothing is different from a user perspective.
Testing Instructions
trunkopen the site editor and dismiss the welcome guideChecklist:
*.native.jsfiles for terms that need renaming or removal).