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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use optional chaining across the entire repo #5609

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

IvanGoncharov
Copy link
Member

Pure mechanical change, shouldn't affect anything 馃檹

Copy link
Member

@benjamn benjamn left a 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.

packages/apollo-datasource-rest/src/RESTDataSource.ts Outdated Show resolved Hide resolved
IvanGoncharov added a commit to IvanGoncharov/apollo-server that referenced this pull request Aug 12, 2021
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,
Copy link
Member

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.

Suggested change
err?.message || err,
err?.message ?? err,

Copy link
Member

@trevor-scheer trevor-scheer left a 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>
@IvanGoncharov IvanGoncharov merged commit 1a92f79 into apollographql:main Aug 13, 2021
@IvanGoncharov IvanGoncharov deleted the useOptionalChaining branch August 13, 2021 13:43
@glasser
Copy link
Member

glasser commented Aug 17, 2021

Did you find these automatically somehow?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants