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

docs: refactored examples that use the logical ! #2602

Merged
merged 2 commits into from
Aug 19, 2022

Conversation

MoustaphaDev
Copy link
Contributor

@MoustaphaDev MoustaphaDev commented Aug 11, 2022

Summary

Using the bang operator (!) on non-boolean values can lead to unexpected results. For example, in the case of this example, using the pause option inside the useQuery hook, one might set the value of the from variable to be 0. In that case the checks would be inaccurate since !0 === true. However, we only expect to pause the query only when from or limit is nullish. We can replace that condition with a more robust one.

Yes, it's an edge case, but new developers reading this documentation might not be aware of the presence of this bug and might have some unexpected results using this example in their code.

Set of changes

Replaced !from || !limit with the more accurate from === undefined || from === null || limit === undefined || limit === null in the useQuery hook/composable on React/Preact Basics and Vue basics.
An intermediate variable shouldPause got used to avoid inlining that long condition into the pause option.

## Summary

Using the bang operator (!) on non-boolean values can lead to unexpected results. For example, in the case of [this example](/basics/react-preact.md#pausing-usequery), using the `pause` option inside the `useQuery` hook, one might set the value of the `from` variable to be `0`. In that case the checks would be inaccurate since `!0 === true`. However, we only expect to pause the query only when `from` or `limit` is nullish. We can replace that condition with a more robust one.

Yes, it's an edge case, but new developers reading this documentation might not be aware of the presence of this bug and might have some unexpected results using this example in their code.

## Set of changes

Replaced `!from || !limit` with the more accurate `from === undefined || from === null || limit === undefined || limit === null` in the `useQuery` hook/composable on [React/Preact Basics](/basics/react-preact.md) and [Vue basics](/basics/vue.md).
An intermediate variable `shouldPause` got used to avoid inlining that long condition into the `pause` option.
@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2022

⚠️ No Changeset found

Latest commit: 8b2f0ca

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@MoustaphaDev MoustaphaDev marked this pull request as ready for review August 11, 2022 22:39
@MoustaphaDev MoustaphaDev changed the title docs:refactored examples using the bang operator docs: refactored examples that use the logical ! Aug 11, 2022
Comment on lines 272 to 273
const shouldPause = computed(() => from === undefined || from === null ||
limit === undefined || limit === null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const shouldPause = computed(() => from === undefined || from === null ||
limit === undefined || limit === null);
const shouldPause = computed(() => from == null || limit == null);

Is a bit more terse 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit 😂
Sacrificing the edge case for a more terse code sounds good to me

Copy link
Contributor Author

@MoustaphaDev MoustaphaDev Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just realized that your suggestion only applied to the basics/vue.md example. In basics/react-preact.md there's a similar example but it still has my initial suggestion. Should I open a new PR to fix that?

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
@kitten kitten merged commit fec1ff4 into urql-graphql:main Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants