-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Extract selectors from useResolveEditedEntity hook #67031
Merged
youknowriad
merged 9 commits into
trunk
from
update/extract-selectors-from-use-resolve-edited-entity
Nov 18, 2024
+184
−210
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ee89dfc
Extract selectors from useResolveEditedEntity hook
youknowriad e6cbc37
Remove duplicated selector in edit-post
youknowriad d32abeb
Normalize page id
youknowriad ca72786
Simplify selector with early return
youknowriad a6db741
Small tweaks
youknowriad c33d560
Memoize selector
youknowriad 99ef52f
normalize resolved template id
youknowriad f66b9c4
Fix e2e tests
youknowriad 98ee372
Use raw selectors for the dependencies
youknowriad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
This file was deleted.
Oops, something went wrong.
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble with this is that the
getDefaultTemplateId
call triggers the resolver and awp/v2/templates/lookup
REST request. But the actual selector doesn't always callgetDefaultTemplateId
. If there is a valid homepage, it returns early and the REST request wasn't needed.Because the
select
is same-store we can solve this by calling the plain selector function in the get-dependants callback. That's exactly what we need: just read the data fromstate
and don't do any side effects.I don't know how we would solve this if the
select
was cross-store. Interesting problem for @ellatrix or @Mamaduka who have plenty of experience working with complex selectors: do we have any existing prior art for this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, I updated here to use the selectors directly, I think that's enough.