-
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
Fix navigate to entity crash in Post Editor for 6.7 #66709
Conversation
Size Change: -22 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
This could make sense to me, and it seems to solve the original issue, but I'd appreciate a review from more experienced folks because I am not familiar with onNavigateToEntityRecord
or the implications it might have.
I'm going to suggest we aim to fix this for 6.7.1. No one feels confident on the impact of the change and we're very close the final release. That said I'd still appreciate reviews and/or someone to take this PR over the line. |
Based on this the main impact of this change is that the function becomes async. This means it will always return a Promise. The callback tends to happen in an event handler (e.g. |
await registry | ||
.resolveSelect( coreStore ) | ||
.getPostType( params.postType ); |
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.
We probably need to catch
any potential errors here so they don't leak out as this function was previously not async.
@ndiego @colorful-tones @kevin940726 - I'd appreciate your feedback regarding my comment 🙏 |
Agreed. We also have a few additional items that have been punted 6.7.1, so ideally, we could get that release out by the end of the year. |
Most of the time This means there is a chance that on a slower connectiion, a user might click on a button and then sit around waiting for something to happen with no feedback whilst the request to resolve the post type happens. This probably isn't great. The only other choice seems to be to resolve all post types in advance in the Post Editor. |
IMO, that is normal behavior on the web when navigating to a different page. If you click a link on a website on a slow connection, you'll get the same behavior, right? |
await registry | ||
.resolveSelect( coreStore ) | ||
.getPostType( params.postType ); |
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.
@getdave, can you explain why we need to pre-resolve this selector?
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.
Yes it would help. Let me see if I can dig this up. We should include information inline in a comment.
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.
I think this is probably fixing a symptom rather than fixing the root issue where we may be using an undefined postType object or something.
I take your point but with standard hyperlinks the browser provides feedback that something is happening. You know the page was requested the browser shows UI to advise "I'm loading the new page". With the example I cited the UI will provide no feedback whatsoever. Rather the button will be clicked and then it will appear as though nothing is happening. |
This was always just a hotfix. Ultimately we can find a more holistic solution. |
What?
Resolves error when programmatically navigating to a custom post type in the Post Editor.
Fixes #64814
Why?
Some additional context and discussion:
How?
Ensures all post types are loaded in the Post Editor before dispatching the request to navigate to that post type.
We load "on demand" to avoid performance overhead of loading all post types in the Post Editor.
Testing Instructions
wp/6.7
there will be an error in the consoleTesting Instructions for Keyboard
Screenshots or screencast