Make Navigation fallback selector private #51413
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
getdave
added
[Type] Code Quality
|
@draganescu I started on this but I'm not sure how to get the |
|
Yes it looks like the private selectors are private in the sense they're not exported, not in the sense we use the locking API. Is it difficult to implement lock/unloc in this PR as well? Can I help? |
|
Yeh I guess I'm not sure how to go about implementing the lock/unlock stuff for Core Data. Other modules have it. Any help much appreciated. |
draganescu
force-pushed
the
update/make-nav-fallback-selectors-private
branch
from
b0c9c8a
to
ddaa659
Compare
|
Size Change: -15.5 kB (-1%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
draganescu
force-pushed
the
update/make-nav-fallback-selectors-private
branch
from
ddaa659
to
ace42c3
Compare
|
The problem here is that it looks like locked selectors do not call their resolvers. |
Ah that's not great. Do you have a code path for that problem? |
draganescu
force-pushed
the
update/make-nav-fallback-selectors-private
branch
from
bf6c954
to
00844b5
Compare
draganescu
requested review from
tellthemachines,
nerrad,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
draganescu
requested review from
jsnajdr and
dmsnell
and removed request for
juanmaguitar
|
Hey peeps, does this need to go into WP 6.3? |
dmsnell
reviewed
Jun 28, 2023
| resolvers, | ||
| store, | ||
| resolversCache | ||
| ); |
There was a problem hiding this comment.
great fix, and thanks for following up on this @draganescu
There was a problem hiding this comment.
Yes, this is probably the first time we're creating a private selector with a resolver 👍 However, this fix won't work when you register a private selector after a store got instantiated:
const store = createReduxStore( name, { ..., resolvers: { foo } );
register( store ); // instance is created and resolvers bound
unlock( store ).registerPrivateSelectors( { foo } );Here you supplied a resolver for foo, registered a private selector foo, but they won't be bound together. You'd have to move the register call after the private registration.
That's why the private selectors are bound lazily. I'll try to do a more perfect fix in a separate PR.
There was a problem hiding this comment.
I specifically moved the registration after to get this effect 🤷🏻
There was a problem hiding this comment.
Yes, you had to move the registration, but you shouldn't have to, that's the bug 🙂
I think this PR is mergeable with the simple resolver fix. The correct solution turns out the be a rather big PR -- although it doesn't change a lot of code, it involves a lot of refactoring.
|
Testing now |
getdave
commented
Jun 28, 2023
There was a problem hiding this comment.
I found (and fixed) a bug with one usage in the Browse Mode sidebar that wasn't unlocked. Other than that this tests well.
@tellthemachines This needs to land in 6.3 🙏
@scruffian I cannot approve this cos I raised the PR so can you ✅ ?
scruffian
requested changes
Jun 28, 2023
|
Flaky tests detected in 06ad9ce. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5400496691
|
|
It seems the update in the The |
I think it's because for private selectors, We could merge #52036 to unblock this. I'm not happy with the PR, but it works. |
I can try to do a minimalistic solution right here in this PR. #52036 is bigger than I originally expected especially because I wanted to add a good unit test, and that requires a working |
tellthemachines
added
the
Backport to WP 6.7 Beta/RC
|
Pushed a fix for the private selectors with resolvers. I also had to remove |
scruffian
approved these changes
Jun 29, 2023
tellthemachines
pushed a commit
that referenced
this pull request
Jul 3, 2023
* Move selector to become private * adds basic lock functionality * remove useless lock-unlock file * map private selectors to resolvers * Unlock the other usage * only create one fallback per session * Fix core-data duplicate private opt-in * Data: bind resolvers to selectors individually, support private selectors --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Jarda Snajdr <jsnajdr@gmail.com>
|
I just cherry-picked this PR to the update/wp6-3-beta3 branch to get it included in the next release: 7b8db97 |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
added a commit
that referenced
this pull request
Jul 3, 2023
* Footnotes: inscrease selector specificity for anchor (#52179) * Patterns: Include template parts for custom areas in Uncategorized category (#52159) * Fix custom patterns console error (#51947) * Fix error with react list key with new custom patterns list in inserter * Update placeholder key * Add comment to explain the different keys * Patterns: Fix missing custom patterns in patterns explorer (#51889) * Add custom patterns to pattern explorer * show custom patterns in the patterns explorer dialog * remove changes from 51877 * Fix up use of async lists * remove a bit of code duplication by adding a new hook * add 51877 fix back to make testing easier * Just assign the key value in one place * Refactor the custom patterns to use the usePatternsState hook * Fix use of async list * Translate strings and remove unneeded fields from pattern object * Try integrating unsynced patterns directly into pattern selectors (#51955) * Include reusable blocks with an undefined sync status in inserter items * Update docs * Remove change to hover dependencies --------- Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * i18n: Add context to the word "Filters" (#52198) * Update home template icon (#52075) * Centralise all permissions lookup in Link UI and enable (#52166) * BlockRemovalWarningModal: Fix incorrect '_n' usage (#52164) * Fix fetching Nav fallback ID flushing Navigation entity cache (#52069) * Only flush the `getEntityRecords` cache if the fallback isn’t already in state * Save the edited entity record to a const and then invert it to determine whether we should invalidate the recordds --------- Co-authored-by: scruffian <ben@scruffian.com> * Block Editor: Unify texts for Create pattern modal (#52151) * Fix history back after entering edit mode from Patterns (#52112) * Add template part icons to the library grid items (#51963) * Patterns: Fix sidebar tab label (#51953) * Patterns: Fix setting of sync status for fully synced patterns (#51952) * Library: Reinstate manage all template parts page (#51961) * Command Palette: fix incorrect path and snackbar message when template part is deleted (#52034) * Command Center: Fix incorrect navigation when deleting template part * removeTemplate: consider title type * Drop-indicator: remove white border. (#52122) * Make Navigation fallback selector private (#51413) * Move selector to become private * adds basic lock functionality * remove useless lock-unlock file * map private selectors to resolvers * Unlock the other usage * only create one fallback per session * Fix core-data duplicate private opt-in * Data: bind resolvers to selectors individually, support private selectors --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Jarda Snajdr <jsnajdr@gmail.com> * Focus Mode: Use the symbol icon if a pattern is being edited (#52031) * Footnotes: register meta field for pages (#52024) * Fix unintentional toggling on of distraction free (#52090) * replace toggle with set preference - because I don't read code properly it seems * remove notification * Revert "Updating social link attributes (#51997)" (#52019) This reverts commit c711e2a. * Update home template name (#52048) * Removes unused call (#51988) * Remove ability for user to toggle sync status after pattern creation (#51998) * Fix disable DFM when opening styles command (#52165) * Update custom patterns label to 'My patterns' (#51949) * rename custom patterns to my patterns * Add my patterns label to inserter and show at the top --------- Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Library: Add sync status to pattern details screen (#51954) * Patterns: Rename Library to Patterns (#52102) * [Library] Add lock icon for theme patterns (#51990) * Add lock icon for theme patterns * Change to class names * Add aria-description * Change wording * Patterns: Use "detached" copy consistently (#51993) * Editor initrial appender: Zero out margins in constrained layouts. (#52026) * Update pattern creation modal in library (#51946) * Fix missing snackbars in Library (#52021) * Make the entire preview clickable in order to enter "edit" mode in focus mode (#51973) * Page Content Focus: Add welcome guides (#52014) * Page Content Focus: Add welcome guides * Don't show when editor guide is active * Just use regular accent/theme color in all guides * slight copy change page guide * Update components changelog * Disable new guides in E2E tests * Use s.w.org videos --------- Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> --------- Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Carolina Nymark <myazalea@hotmail.com> Co-authored-by: Joen A <1204802+jasmussen@users.noreply.github.com> Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Jarda Snajdr <jsnajdr@gmail.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Robert Anderson <robert@noisysocks.com>
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Move selector to become private * adds basic lock functionality * remove useless lock-unlock file * map private selectors to resolvers * Unlock the other usage * only create one fallback per session * Fix core-data duplicate private opt-in * Data: bind resolvers to selectors individually, support private selectors --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Jarda Snajdr <jsnajdr@gmail.com>
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.
What?
Makes the selector added for Navigation fallbacks private.
Closes #50372
Why?
Because we're not 100% confident on the API yet.
How?
core-datapackagedatapackage to map private selectors to their resolversTesting Instructions
getNavigationFallbackIdshould not accessible without unlocking the core store