-
Notifications
You must be signed in to change notification settings - Fork 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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use optional chaining across the entire repo #5609
Use optional chaining across the entire repo #5609
Conversation
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.
These changes looks good to me, though I found one spot where the original code was wrong, in a way that means chaining should not be necessary.
It should safe to use `typeof` & `instanceof` on any value including `null` and `undefined`. Also `isObjectType` accepts any value so no need to prefix it with nullable check. I spot these places during apollographql#5609
@@ -217,7 +217,7 @@ export async function processGraphQLRequest<TContext>( | |||
} catch (err) { | |||
logger.warn( | |||
'An error occurred while attempting to read from the documentStore. ' + | |||
(err && err.message) || err, | |||
err?.message || err, |
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.
Would you be interested in updating these usages of ||
to the null coalesce ??
operator as well? Keeping in mind the slight functional difference between the two (i.e. if err?.message === ''
). Possibly a separate PR.
err?.message || err, | |
err?.message ?? err, |
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, barring @benjamn's suggestion and whether or not you'd like to also tackle the ??
update as well.
Co-authored-by: Ben Newman <ben@benjamn.com>
Did you find these automatically somehow? |
Pure mechanical change, shouldn't affect anything 馃檹