-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Time to Read block showing "this block has encountered an error" - #61459 #61614
Conversation
…h optional chaining so undefined can be prevented
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @bradhogan. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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.
Thanks for the PR!
This PR makes changes to the utility functions in the wordcount
package, but as the JSDoc shows, these utility functions are not expected to be passed undefined text, so unexpected problems may occur.
I think this problem needs to be solved on the block side. I guess the fundamental problem is that content
becomes undefined
here:
wordCount( content, wordCountType ) / AVERAGE_READING_RATE |
I haven't tested it, but could it be resolved by passing an empty string as a fallback as shown below?
diff --git a/packages/block-library/src/post-time-to-read/edit.js b/packages/block-library/src/post-time-to-read/edit.js
index 5cdb81c05e..abfdce6630 100644
--- a/packages/block-library/src/post-time-to-read/edit.js
+++ b/packages/block-library/src/post-time-to-read/edit.js
@@ -66,7 +66,7 @@ function PostTimeToReadEdit( { attributes, setAttributes, context } ) {
const minutesToRead = Math.max(
1,
Math.round(
- wordCount( content, wordCountType ) / AVERAGE_READING_RATE
+ wordCount( content || '', wordCountType ) / AVERAGE_READING_RATE
)
);
Thanks @t-hamano, I have'nt looked at the JS Doc comment, hence routed that approach. Would look at the patch above and would make the changes to PR asap. |
Hello @t-hamano, have tested the changes on the local and yes, the content is undefined at that point which results in the error. Have updated the PR with description and required changes. Thank You. |
Hi @t-hamano, Just a quick question, this function may be used else whereas well, so we need to update this type of issue at other places too (if present). I am curious can't we just update all the functions with the optional chaining, so that we do not change on every place, instead we just change the function itself? Thank You 🙏. |
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.
LGTM!
I am curious can't we just update all the functions with the optional chaining, so that we do not change on every place, instead we just change the function itself?
I think the logic in the wordcount
package's utility functions is based on the assumption that some string exists. Therefore, if we allow undefined
value, I think we will get a result that is not calculated correctly.
It may be good idea to implement the safeguards you suggest in the future, but for now I think it's enough to just fix the block side.
What?
Why?
wordCount
function if we get the content as undefined.How?
content
should not be passed as undefined to the function.Testing Instructions
post-time-to-read
block.Testing Instructions for Keyboard
Screenshots or screencast
GB.Issue.-.61459.webm