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

fix: Add noUncheckedIndexAccess to tsconfig #281

Merged
merged 8 commits into from
Nov 15, 2024
Merged

fix: Add noUncheckedIndexAccess to tsconfig #281

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
18d9bbd
Turn on the setting, there are 10 errors or so
lanesawyer Nov 3, 2024
d25812f
Merge branch 'main' into lane/213-ts-noUncheckedIndexAccess
lanesawyer Nov 13, 2024
effaf1e
Merge branch 'main' into lane/213-ts-noUncheckedIndexAccess
lanesawyer Nov 15, 2024
029cef2
Working on some fixes
lanesawyer Nov 15, 2024
9f8ec88
Fix remaining issues
lanesawyer Nov 15, 2024
c82d198
fix bad variable name
lanesawyer Nov 15, 2024
d89d8ee
Merge branch 'main' into lane/213-ts-noUncheckedIndexAccess
lanesawyer Nov 15, 2024
a5c4d33
Address PR feedback
lanesawyer Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libs/isograph-react/src/core/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ function normalizeLinkedField(
const dataIds: (Link | null)[] = [];
for (let i = 0; i < networkResponseData.length; i++) {
const networkResponseObject = networkResponseData[i];
if (networkResponseObject === null) {
if (networkResponseObject == null) {
dataIds.push(null);
continue;
}
Expand Down
21 changes: 16 additions & 5 deletions libs/isograph-react/src/core/read.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

throw new Error('refetchQuery is null in RefetchField');
throw new Error('refetchQueryIndex is null in RefetchField');
}
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!

}
const refetchQueryArtifact = refetchQuery.artifact;
const allowedVariables = refetchQuery.allowedVariables;

Expand All @@ -371,9 +374,13 @@ function readData<TReadFromStore>(
}
case 'Resolver': {
const usedRefetchQueries = field.usedRefetchQueries;
const resolverRefetchQueries = usedRefetchQueries.map(
(index) => nestedRefetchQueries[index],
);
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!

}
return resolverRefetchQuery;
});

switch (field.readerArtifact.kind) {
case 'EagerReaderArtifact': {
Expand Down Expand Up @@ -636,7 +643,11 @@ function generateChildVariableMap(
const childVars: Writable<Variables> = {};
for (const [name, value] of fieldArguments) {
if (value.kind === 'Variable') {
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!

}
childVars[name] = variable;
} else {
childVars[name] = value.value;
}
Expand Down
23 changes: 17 additions & 6 deletions libs/isograph-react/src/loadable-hooks/useConnectionSpecPagination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

}

const firstParameter = {
data: readOutDataAndRecords[i].item,
data,
parameters: fragmentReference.variables,
};

Expand Down Expand Up @@ -165,10 +170,17 @@ export function useConnectionSpecPagination<
fragmentReference.readerWithRefetchQueries,
);

const records = readOutDataAndRecords[i];
if (records == null) {
throw new Error(
'subscribeCompletedFragmentReferences records is unexpectedly null',
);
}

return {
fragmentReference,
readerAst: readerWithRefetchQueries.readerArtifact.readerAst,
records: readOutDataAndRecords[i],
records,
callback(_data) {
rerender({});
},
Expand Down Expand Up @@ -224,10 +236,9 @@ export function useConnectionSpecPagination<

const loadedReferences = state === UNASSIGNED_STATE ? [] : state;

const mostRecentItem: LoadedFragmentReference<
TReadFromStore,
Connection<TItem>
> | null = loadedReferences[loadedReferences.length - 1];
const mostRecentItem:
| LoadedFragmentReference<TReadFromStore, Connection<TItem>>
| undefined = loadedReferences[loadedReferences.length - 1];
const mostRecentFragmentReference =
mostRecentItem?.[0].getItemIfNotDisposed();

Expand Down
21 changes: 17 additions & 4 deletions libs/isograph-react/src/loadable-hooks/useSkipLimitPagination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,13 @@ export function useSkipLimitPagination<
fragmentReference.readerWithRefetchQueries,
);

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

const firstParameter = {
data: readOutDataAndRecords[i].item,
data,
parameters: fragmentReference.variables,
};

Expand Down Expand Up @@ -150,10 +155,17 @@ export function useSkipLimitPagination<
fragmentReference.readerWithRefetchQueries,
);

const records = readOutDataAndRecords[i];
if (records == null) {
throw new Error(
'subscribeCompletedFragmentReferences records is unexpectedly null',
);
}

return {
fragmentReference,
readerAst: readerWithRefetchQueries.readerArtifact.readerAst,
records: readOutDataAndRecords[i],
records,
callback(_data) {
rerender({});
},
Expand Down Expand Up @@ -209,8 +221,9 @@ export function useSkipLimitPagination<

const loadedReferences = state === UNASSIGNED_STATE ? [] : state;

const mostRecentItem: LoadedFragmentReference<TReadFromStore, TItem> | null =
loadedReferences[loadedReferences.length - 1];
const mostRecentItem:
| LoadedFragmentReference<TReadFromStore, TItem>
| undefined = loadedReferences[loadedReferences.length - 1];
const mostRecentFragmentReference =
mostRecentItem?.[0].getItemIfNotDisposed();

Expand Down
1 change: 1 addition & 0 deletions tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"noPropertyAccessFromIndexSignature": true,
"noUnusedParameters": true,
"exactOptionalPropertyTypes": true,
"noUncheckedIndexedAccess": true,
"paths": {
"@isograph/*": ["./libs/*"]
},
Expand Down
Loading