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.

Sign up for GitHub

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

Jump to bottom

Enable noUncheckedIndexedAccess #3867

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Enable noUncheckedIndexedAccess #3867

wants to merge 4 commits into from

Conversation

louisscruz
Copy link

@louisscruz louisscruz commented Mar 27, 2023 •
edited
Loading

I'm interested in possibly contributing to graphql in the future. I noticed that noUncheckedIndexedAccess is not yet enabled and that there was an associated FIXME comment in the configuration. Because it would make contributions safer in the future, and because I wanted a kind of tour of the code in this package, I turned on noUncheckedIndexedAccess and fixed all of the violations.

The vast majority of violations were fixed with either assert or invariant. In some cases, fixes were simple in that they only required different iterating mechanisms to be done in a more type safe way.

General comments/questions:

  • Could any of the code in this PR break in some environments? I'm not sure which browsers/Node versions are supported here. I think the most advanced API I used was Object.entries, but that seems to be compatible back to Node 7. Any reviews with this in mind would be good.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 27, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented Mar 27, 2023 •
edited
Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 7cc264d
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/642770c88b631c000806df21
😎 Deploy Preview https://deploy-preview-3867--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@louisscruz louisscruz marked this pull request as ready for review March 27, 2023 01:11
@github-actions
Copy link

Hi @louisscruz, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pull_request.yml:codeql. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@IvanGoncharov
Copy link
Member

@louisscruz Thanks for PR 👍
I quickly look through it and I will try to give you a partial review tomorrow.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Mar 31, 2023
@IvanGoncharov
Enable @typescript-eslint/strict-boolean-expressions rule
528d67a 
Motivation: Fix edge cases like in graphql#3869
Also I notice similar issue in graphql#3867 so I decided to fix it for the entire codebase.
@IvanGoncharov IvanGoncharov mentioned this pull request Mar 31, 2023
Merged
@louisscruz louisscruz force-pushed the lcruz/no-unchecked-indexed-access branch from 8d5f1ee to 7cc264d Compare March 31, 2023 23:46
@louisscruz
Copy link
Author

@IvanGoncharov Just giving a ping on this.

I just rebased to address a conflict. I noticed the use of delegating multiple checks to single invariant calls. I can make a pass to do that in this PR if you'd like. Seems nicer.

IvanGoncharov added a commit that referenced this pull request Apr 1, 2023
@IvanGoncharov
Enable @typescript-eslint/strict-boolean-expressions rule (#3872)
8c749e9 
Motivation: Fix edge cases like in #3869
Also, I noticed a similar issue in #3867, so I fixed it for the entire codebase.

P.S. I also resulted in a small but measurable speedup, probably because I replaced a bunch of ObjMap with ES6 Map.
@yaacovCR
Copy link
Contributor

I guess we were not ready to be that strict yet.

I have been reviewing old PRs, and am just noting that #3504 might be a useful addition to this PR, but I think in general this PR would require careful review to ensure that it does not introduce unnecessary runtime checks that can be avoided in similar ways to #3504

@louisscruz
Copy link
Author

louisscruz commented Oct 25, 2024 •
edited
Loading

If it'd still be welcome to make this change, I'd be happy to get this PR back up to a working state. And yeah, happy to remove any runtime checks that can be avoided through other means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@louisscruz @IvanGoncharov @yaacovCR