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

fix: Add noUncheckedIndexAccess to tsconfig #281

Merged

Conversation

lanesawyer
Copy link
Collaborator

@lanesawyer lanesawyer commented Nov 15, 2024

fix: Add noUncheckedIndexAccess to tsconfig

What

How

  • Added line to the file
  • Fixed issues primarily by throwing errors, as each area of the code expects it to exist and something is very wrong if it doesn't
  • Loosened equality checks in some places so all falsey values are caught
  • Changed a few nulls to undefineds because accessing an index that doesn't exist (or accessing an array with undefined as an index value returns undefined)

@lanesawyer
Copy link
Collaborator Author

@rbalicki2 Did my best on the error throwing messages, let me know if you have more informative ones in mind!

Copy link
Collaborator

@rbalicki2 rbalicki2 left a comment

Choose a reason for hiding this comment

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

just a few nits, overall, amazing!!! Love it. Thank you for making the codebase stricter and saving my future self from my now self

const resolverRefetchQueries = usedRefetchQueries.map((index) => {
const resolverRefetchQuery = nestedRefetchQueries[index];
if (resolverRefetchQuery == null) {
throw new Error('resolverRefetchQuery is null in Resolver');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add this is indicative of a bug in Isograph?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@@ -343,9 +343,12 @@ function readData<TReadFromStore>(
} else {
const refetchQueryIndex = field.refetchQuery;
if (refetchQueryIndex == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

oops, this can never be null... the field is not optional. This is probably a holdover from a previous time. perhaps something like https://typescript-eslint.io/rules/no-unnecessary-condition/ can prevent this in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed!

}
const refetchQuery = nestedRefetchQueries[refetchQueryIndex];
if (refetchQuery == null) {
throw new Error('refetchQuery is null in RefetchField');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add that this is indicative of a bug in Isograph

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

childVars[name] = variables[value.name];
const variable = variables[value.name];
if (variable == null) {
throw new Error('Variable ' + value.name + ' is not missing');
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/not missing/missing

But also, this breaks if we have a default value for a variable and the variable is not provided, e.g. field Query.PetDetailRoute($id: ID = 4) @component { with no variable provided.

I think the appropriate behavior is to continue the loop, i.e. we don't (currently) distinguish between missing and null variables, so skipping the variable is appropriate (and based on my limited testing, works)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh ChatGPT, thank you for being awful lol. I did not even see the "not" lol

And that logic makes sense! Will just allow the loop to continue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@@ -114,8 +114,13 @@ export function useConnectionSpecPagination<
fragmentReference.readerWithRefetchQueries,
);

const data = readOutDataAndRecords[i]?.item;
if (data == null) {
throw new Error('Parameter data is unexpectedly null');
Copy link
Collaborator

Choose a reason for hiding this comment

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

indicative of a bug in isograph, likewise below and in useSkipLimit. Could you also add a comment // invariant: readOutDataAndRecords.length === completedReferences.length somewhere near here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

@rbalicki2 rbalicki2 left a comment

Choose a reason for hiding this comment

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

Looks amazing!! Shipitttt

@rbalicki2 rbalicki2 merged commit 4941e5b into isographlabs:main Nov 15, 2024
20 checks passed
@lanesawyer lanesawyer deleted the lane/213-ts-noUncheckedIndexAccess branch November 15, 2024 05:52
PatrykWalach pushed a commit to PatrykWalach/isograph that referenced this pull request Dec 21, 2024
# fix: Add noUncheckedIndexAccess to tsconfig

## What
- Adds the `noUncheckedIndexAccess` option to the base `tsconfig.json` file
- Fixes the errors that appeared
- Resolves isographlabs#213 

## How
- Added line to the file
- Fixed issues primarily by throwing errors, as each area of the code expects it to exist and something is very wrong if it doesn't
- Loosened equality checks in some places so all falsey values are caught
- Changed a few `null`s to `undefined`s because accessing an index that doesn't exist (or accessing an array with `undefined` as an index value returns `undefined`)
PatrykWalach pushed a commit to PatrykWalach/isograph that referenced this pull request Jan 6, 2025
# fix: Add noUncheckedIndexAccess to tsconfig

## What
- Adds the `noUncheckedIndexAccess` option to the base `tsconfig.json` file
- Fixes the errors that appeared
- Resolves isographlabs#213 

## How
- Added line to the file
- Fixed issues primarily by throwing errors, as each area of the code expects it to exist and something is very wrong if it doesn't
- Loosened equality checks in some places so all falsey values are caught
- Changed a few `null`s to `undefined`s because accessing an index that doesn't exist (or accessing an array with `undefined` as an index value returns `undefined`)
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.

[typescript] Turn on all of the recommended config options
2 participants