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

chore(docs): use optional chaining rather than non-null assertion #10129

Conversation

charpeni
Copy link
Contributor

While reading through the excellent documentation of useFragment_experimental, I noticed that the example using useQuery uses the non-null assertion operator as a way to assert that data can't be null in this context, which is unfortunately not the case.

I think this usage could be confusing as the code snippet mentioned JavaScript, but that operator won't have any effects outside of TypeScript as the operator is simply removed in the emitted JavaScript code. Even with TypeScript, if someone copy-paste that example, it could lead to a runtime error are we're telling the compiler that data is a known non-null value, but we never asserted it was the case. If we omit it, then TypeScript will show the following error: Cannot read properties of undefined (reading 'list'), but since we're using the non-null assertion operator, we're just muting the error, which is leading to a runtime error.

I think it could make sense to use the non-null assertion operator combined with strictNullChecks, but only if we're making sure to assert it before! E.g.:

if (loading) {
  return;
}

return (
  {data!.list.map(item => <Item key={item.id} id={item.id}/>)}
  {/* ^ We previously asserted that `loading` couldn't be true here, therefore, `data` must be defined. */}
);

The current code sample is failing with a runtime error with both JavaScript and TypeScript, and to remove some confusion for readers that could be wondering why they need the non-null assertion operator (post-fix !), I would suggest we simplify this by using the optional chaining operator (?.) instead.

cc @alessbell

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@alessbell
Copy link
Member

Thanks for the PR and good catch, @charpeni!

I totally agree, that example code should be using optional chaining for the reasons you cited. Will merge this in 👍

@alessbell alessbell merged commit 74dbb99 into apollographql:main Sep 26, 2022
@charpeni charpeni deleted the usefragment-non-null-assertion-operator-usage branch September 26, 2022 19:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 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

2 participants