Skip to content
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

Data: Log first-pass `mapSelect` errors. #20122

Merged
merged 1 commit into from Feb 10, 2020
Merged

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Feb 9, 2020

Closes #20117

This PR makes useSelect log the first error thrown in mapSelect instead of just swallowing it and waiting for a second one. This doesn't decrease the durability of the hook against zombie child updates. Still, it allows for easier debugging of errors like the one in #20117 where a first-pass render exception was causing the hook to return undefined, and that triggered an obscure error elsewhere.

@@ -114,6 +114,9 @@ export default function useSelect( _mapSelect, deps ) {
errorMessage += 'Original stack trace:';

throw new Error( errorMessage );
} else {
// eslint-disable-next-line no-console
console.error( errorMessage );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Feb 9, 2020

Member

Instead of logging the error could we do throw new Error( errorMessage );? If yes we can simply remove the else and move throw new Error( errorMessage ); outside the if.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 9, 2020

Author Contributor

No, that would remove the protection against zombie child updates.

@ellatrix ellatrix removed this from the Gutenberg 7.5 milestone Feb 10, 2020
@nerrad
nerrad approved these changes Feb 10, 2020
Copy link
Contributor

nerrad left a comment

I think doing a console.error here is a sufficient way to surface errors from mapSelect on the first render calculation. This is a tricky one to mitigate because:

  • useSelect implementations can return any type from mapSelect
  • the onus is on implementations to be defensive against unexpected errors within the callbacks.

So I think the most we can do here (with the current logic) is to surface errors in the console as Enrique has done. Looks good to me!

I think I recall seeing an a new warn package implemented recently, should we use that instead of console directly here?

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 10, 2020

@epiqueras what about utilizing this new package added recently? https://github.com/WordPress/gutenberg/tree/master/packages/warning

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 10, 2020

It doesn't log in production.

@epiqueras epiqueras merged commit 0e0951d into master Feb 10, 2020
3 checks passed
3 checks passed
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the add/map-select-debug-info branch Feb 10, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Tightening Up
  
Awaiting triage
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.