Remaining feedback from #622 review #624
Comments
|
glasser
added a commit
that referenced
this issue
Apr 8, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at least non-ideal error-handling cases) that arose from TypeScript's default behavior of assuming that all index operations yield valid values. That is, by default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y` rather than `Y | undefined`, and it assumes that all array access attempts are in bounds. noUncheckedIndexedAccess changes this so that you generally need to actually make sure that your indexes worked. (This is a bit awkward for arrays because it doesn't let you rely on `.length` checks, but it's not the end of the world.) More details on the flag are available at microsoft/TypeScript#39560 Here's an overview of the sorts of changes involved: - One of the issues tracked in #624 is that buildComposedSchema could fail with unclear errors if the input schema doesn't declare directives correctly; the code assumed that any `@join__owner` or `@join__type` directive application *must* have a non-null `graph` argument whose value matches one of the `join__Graph` enums. We probably should validate the definitions, but in the meantime this PR validates that the value we get out of the directive application is good, with explicit errors if not. - Our code uses a lot of record types like `{[x: string]: Y}`. noUncheckedIndexedAccess means we need to actually check that the value exists. (Another alternative would be to change to `Map` which does not have this issue.) I added a helper `getOrSetRecord` which allowed us to express a very common case more succinctly, which also led to less duplication of code. - In some cases where it is very clear from context that a lookup must succeed (eg, a length check), I just used `!`. - Some cases in composition explicitly checked to see if directives had arguments but not if it had at least one argument; adding a `?.[0]` helped here. - Many cases were fixed by just using `?.` syntax or saving a lookup in a variable before moving on. - Iteration over `Object.keys` could be replaced by `Object.values` or `Object.entries` to eliminate the index operation. - In some cases we could change the declaration of array types to be "non-empty array" declarations, which look like `[A, ...A[]]`. For example, the groupBy operation always makes the values of the returned map be non-empty arrays. I was also able to replace a bunch of the FieldSet types in the query planner with a NonemptyFieldSet type; there are other places where it might be good to improve our checks for nonempty FieldSets though. - Nested `function`s that close over values in their surrounding scope can't rely on narrowing of those values, but arrow functions assigned to a `const` that happen after the narrowing can, so in some cases I replaced nested functions with arrow functions (which generally involved moving it around too).
glasser
added a commit
that referenced
this issue
Apr 9, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at least non-ideal error-handling cases) that arose from TypeScript's default behavior of assuming that all index operations yield valid values. That is, by default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y` rather than `Y | undefined`, and it assumes that all array access attempts are in bounds. noUncheckedIndexedAccess changes this so that you generally need to actually make sure that your indexes worked. (This is a bit awkward for arrays because it doesn't let you rely on `.length` checks, but it's not the end of the world.) More details on the flag are available at microsoft/TypeScript#39560 Here's an overview of the sorts of changes involved: - One of the issues tracked in #624 is that buildComposedSchema could fail with unclear errors if the input schema doesn't declare directives correctly; the code assumed that any `@join__owner` or `@join__type` directive application *must* have a non-null `graph` argument whose value matches one of the `join__Graph` enums. We probably should validate the definitions, but in the meantime this PR validates that the value we get out of the directive application is good, with explicit errors if not. - Our code uses a lot of record types like `{[x: string]: Y}`. noUncheckedIndexedAccess means we need to actually check that the value exists. (Another alternative would be to change to `Map` which does not have this issue.) I added a helper `getOrSetRecord` which allowed us to express a very common case more succinctly, which also led to less duplication of code. - In some cases where it is very clear from context that a lookup must succeed (eg, a length check), I just used `!`. - Some cases in composition explicitly checked to see if directives had arguments but not if it had at least one argument; adding a `?.[0]` helped here. - Many cases were fixed by just using `?.` syntax or saving a lookup in a variable before moving on. - Iteration over `Object.keys` could be replaced by `Object.values` or `Object.entries` to eliminate the index operation. - In some cases we could change the declaration of array types to be "non-empty array" declarations, which look like `[A, ...A[]]`. For example, the groupBy operation always makes the values of the returned map be non-empty arrays. I was also able to replace a bunch of the FieldSet types in the query planner with a NonemptyFieldSet type; there are other places where it might be good to improve our checks for nonempty FieldSets though. - Nested `function`s that close over values in their surrounding scope can't rely on narrowing of those values, but arrow functions assigned to a `const` that happen after the narrowing can, so in some cases I replaced nested functions with arrow functions (which generally involved moving it around too).
17 tasks
glasser
added a commit
that referenced
this issue
Apr 16, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at least non-ideal error-handling cases) that arose from TypeScript's default behavior of assuming that all index operations yield valid values. That is, by default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y` rather than `Y | undefined`, and it assumes that all array access attempts are in bounds. noUncheckedIndexedAccess changes this so that you generally need to actually make sure that your indexes worked. (This is a bit awkward for arrays because it doesn't let you rely on `.length` checks, but it's not the end of the world.) More details on the flag are available at microsoft/TypeScript#39560 Here's an overview of the sorts of changes involved: - One of the issues tracked in #624 is that buildComposedSchema could fail with unclear errors if the input schema doesn't declare directives correctly; the code assumed that any `@join__owner` or `@join__type` directive application *must* have a non-null `graph` argument whose value matches one of the `join__Graph` enums. We probably should validate the definitions, but in the meantime this PR validates that the value we get out of the directive application is good, with explicit errors if not. - Our code uses a lot of record types like `{[x: string]: Y}`. noUncheckedIndexedAccess means we need to actually check that the value exists. (Another alternative would be to change to `Map` which does not have this issue.) I added a helper `getOrSetRecord` which allowed us to express a very common case more succinctly, which also led to less duplication of code. - In some cases where it is very clear from context that a lookup must succeed (eg, a length check), I just used `!`. - Some cases in composition explicitly checked to see if directives had arguments but not if it had at least one argument; adding a `?.[0]` helped here. - Many cases were fixed by just using `?.` syntax or saving a lookup in a variable before moving on. - Iteration over `Object.keys` could be replaced by `Object.values` or `Object.entries` to eliminate the index operation. - In some cases we could change the declaration of array types to be "non-empty array" declarations, which look like `[A, ...A[]]`. For example, the groupBy operation always makes the values of the returned map be non-empty arrays. I was also able to replace a bunch of the FieldSet types in the query planner with a NonemptyFieldSet type; there are other places where it might be good to improve our checks for nonempty FieldSets though. - Nested `function`s that close over values in their surrounding scope can't rely on narrowing of those values, but arrow functions assigned to a `const` that happen after the narrowing can, so in some cases I replaced nested functions with arrow functions (which generally involved moving it around too).
glasser
added a commit
that referenced
this issue
Apr 16, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at least non-ideal error-handling cases) that arose from TypeScript's default behavior of assuming that all index operations yield valid values. That is, by default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y` rather than `Y | undefined`, and it assumes that all array access attempts are in bounds. noUncheckedIndexedAccess changes this so that you generally need to actually make sure that your indexes worked. (This is a bit awkward for arrays because it doesn't let you rely on `.length` checks, but it's not the end of the world.) More details on the flag are available at microsoft/TypeScript#39560 Here's an overview of the sorts of changes involved: - One of the issues tracked in #624 is that buildComposedSchema could fail with unclear errors if the input schema doesn't declare directives correctly; the code assumed that any `@join__owner` or `@join__type` directive application *must* have a non-null `graph` argument whose value matches one of the `join__Graph` enums. We probably should validate the definitions, but in the meantime this PR validates that the value we get out of the directive application is good, with explicit errors if not. - Our code uses a lot of record types like `{[x: string]: Y}`. noUncheckedIndexedAccess means we need to actually check that the value exists. (Another alternative would be to change to `Map` which does not have this issue.) I added a helper `getOrSetRecord` which allowed us to express a very common case more succinctly, which also led to less duplication of code. - In some cases where it is very clear from context that a lookup must succeed (eg, a length check), I just used `!`. - Some cases in composition explicitly checked to see if directives had arguments but not if it had at least one argument; adding a `?.[0]` helped here. - Many cases were fixed by just using `?.` syntax or saving a lookup in a variable before moving on. - Iteration over `Object.keys` could be replaced by `Object.values` or `Object.entries` to eliminate the index operation. - In some cases we could change the declaration of array types to be "non-empty array" declarations, which look like `[A, ...A[]]`. For example, the groupBy operation always makes the values of the returned map be non-empty arrays. I was also able to replace a bunch of the FieldSet types in the query planner with a NonemptyFieldSet type; there are other places where it might be good to improve our checks for nonempty FieldSets though. - Nested `function`s that close over values in their surrounding scope can't rely on narrowing of those values, but arrow functions assigned to a `const` that happen after the narrowing can, so in some cases I replaced nested functions with arrow functions (which generally involved moving it around too).
trevor-scheer
added a commit
that referenced
this issue
Apr 22, 2021
This commit is a set of follow-ups from #622 (and captured in #624). Most of these changes are internal and of little to no outward consequence, however a few changes worth noting are: * a switch to graphql-js's `stripIgnoredCharacters` during field set printing * an update to the `join__Enum` generation algorithm * the splitting of entity and value type metadata types * a conversion of the `GraphMap` type to an actual `Map` * additional assertions throughout
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Navigating PR reviews can be a pain and I actually started making this list just to verify what improvements @martijnwalraven and @trevor-scheer had done after my review. Maybe it is useful to share this here and we can use it to track what things have been done before and after merge and preview etc? (Some of these will probably just be "file another issue to actually track it".) If this is helpful, yay, and if not, feel free to close.
_1[ ] change composition (and spec) to only allow one(Martijn: I don't believe this is what we want)@join__typeper graphundefinedfrom it and improve error messages in the places that this change makes not compileFieldSetprintWithReducedWhitespaceencouraging us to considerstripIgnoredCharacters?The text was updated successfully, but these errors were encountered: