-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(api-graphql): custom operation selection set on nested custom types #13078
Conversation
c56af72
to
43e9b23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I think all of the below are more nit than concern. Good to see this ship as is or with any attention to these comments.
return `{${selectionSetIRToString( | ||
defaultSelectionSetForNonModelWithIR(nonModel, modelIntrospection), | ||
)}}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exporting the default function to call it in passed into IRToString function makes me wonder if it would be better to export a function that combines the two functions instead of stacking the calls here.
import { isApiGraphQLConfig } from '../utils/isApiGraphQLProviderConfig'; | ||
import { generateEnumsProperty } from '../utils/generateEnumsProperty'; | ||
import { isApiGraphQLConfig } from '../utils/runtimeTypeGuards/isApiGraphQLProviderConfig'; | ||
import { generateEnumsProperty } from '../utils/clientProperties/generateEnumsProperty'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of grouping utils up into folders like this. Would it be valuable to have a folder level index that exports the sub-types instead of deep referencing them?
const model = modelIntrospection.nonModels[operation.type.nonModel]; | ||
return `{${modelishTypeSelectionSet(model)}}`; | ||
const nonModel = modelIntrospection.nonModels[operation.type.nonModel]; | ||
return `{${selectionSetIRToString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're wrapping strings in surrounding curly brackets, {}
, in several places. I find the character sequence `{${
a bit hard to read, which makes me wonder if a few extra chars for wrapInCurlyBrackets(x)
might be more readable.
Not specific to this PR, but a thing I thought of.
702f73f
to
e2d6f75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
As Aaron suggested, would be nice to import from a top-level utils/index.ts
instead of sub-dirs, but that's non-blocking
Thanks if this is non-blocking I will open a separate PR to refactor further (also wanted to extra the selection set utils out from that long source file) |
e2d6f75
to
2b026c2
Compare
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.