[TS] Dedup enums and inputs by using global types file#520
[TS] Dedup enums and inputs by using global types file#520shadaj merged 4 commits intoapollographql:masterfrom
Conversation
ghost
added
the
shadaj
left a comment
shadaj
left a comment
There was a problem hiding this comment.
First pass. Changes look good but we could do a bit of cleanup to make them even more awesome!
| case 'Field': | ||
| case 'TypeCondition': | ||
| if (selection.selectionSet) { | ||
| selectionAcc = reduceTypesForDocument(selection.selectionSet, selectionAcc); |
There was a problem hiding this comment.
Updating selectionAcc and returning later introduces a bit of unnecessary mutability. Could you just directly return the additional types here and return selectionAcc for the default case?
| type = type.ofType | ||
| } | ||
|
|
||
| acc = maybePush(acc, type); |
There was a problem hiding this comment.
Same as above, could this be replaced by an immutable update [...acc, type]?
There was a problem hiding this comment.
As far as I can tell, the intention of maybePush from the original reduceTypesUsed was that there are no duplicated types in acc as maybePush checks for dups. However since we filter later anyway, we could accumulate duplicates here. Or we could collect them as an object instead thus having O(1) later for the lookups.
| type = getNullableType(type); | ||
| } | ||
|
|
||
| if (type instanceof GraphQLList) { |
There was a problem hiding this comment.
What happens with nested lists here? This might need a recursive call so the body of this branch would be something like return reduceGlobalTypesUsed(acc, type.ofType).
There was a problem hiding this comment.
Yeah, this currently fails on [[EnumCommentTestCase!]!]!. Added a test case.
| acc: (GraphQLType | GraphQLOutputType)[], | ||
| type: GraphQLType | ||
| ) =>{ | ||
| if (type instanceof GraphQLNonNull) { |
There was a problem hiding this comment.
Same as below, probably best to just recurse in case you have a non-nullable list for example.
There was a problem hiding this comment.
Yeah, this currently fails on [[EnumCommentTestCase!]!]!. Added a test case.
| public getGlobalTypesUsedForOperation(doc: Operation | Fragment, context: CompilerContext) { | ||
| let docTypesUsed: GraphQLType[] = []; | ||
|
|
||
| if (doc.hasOwnProperty('operationName')) { |
There was a problem hiding this comment.
Could this be replaced by an instanceof or kind check? Then it wouldn't be necessary to explicitly cast the value later.
| nested: SelectionSet, | ||
| acc: GraphQLType[] | ||
| ) => { | ||
| acc = nested.selections |
There was a problem hiding this comment.
Instead of setting and immediately returning acc, can you just directly return it here?
|
|
||
| if (nextToSources || (fs.existsSync(outputPath) && fs.statSync(outputPath).isDirectory())) { | ||
| if (nextToSources && !fs.existsSync(outputPath)) { | ||
| fs.mkdirSync(outputPath); |
There was a problem hiding this comment.
In this case you want to be checking and creating the joined path, right? I think you might need to move the dir adjustment logic from below up to the top of this branch so that you are using the same directory everywhere. Plus, it's expensive to check directories for every file so it would be faster to just do that once here.
There was a problem hiding this comment.
I am not sure I understand what you are trying to say. This line would create the directory for globalTypes.ts if we are placing next to sources. In that case we are not guaranteed to have the /__generated__ (or whatever it is set to) directory (as there is no source next to it), so its checked here. The later dir logic is for the directories we need for placing next to the source. That part is just copied from the flow target. If instead we are using output flat, no checks or creates would happen except the initial one (on Line 129).
There was a problem hiding this comment.
Sorry, I was confusing sourcePath and outputPath. You're right, this makes sense for the globalTypes.ts file.
| [fileName: string]: BasicGeneratedFile; | ||
| } = {}; | ||
|
|
||
| if (nextToSources || (fs.existsSync(outputPath) && fs.statSync(outputPath).isDirectory())) { |
There was a problem hiding this comment.
This logic still seems pretty similar to Flow other than the change in the call to content. Would it be possible to extract out the common logic?
There was a problem hiding this comment.
In fact, you could probably use the TypeScript code for Flow as is. The Flow generator would receive the additional property globalSourcePath but it would just ignore it.
There was a problem hiding this comment.
So the separation was with intent. Yes there is a lot of logic, but there is also apollographql/rover#410 which I didn't want to interfere with. I wanted to keep them separate until Flow catches up and dedups as well. Then those paths could be merged again (if similar enough). What do you think?
If we use the TS code as is for flow, we would also have to modify the flow package to return content as a closure. (Which I am not really interested in doing to be honest as I don't care much about Flow.)
|
Thanks for the thorough review @shadaj ! Initially I just copied the code for |
|
Looks good to me, thanks for the awesome work @danilobuerger! |
2 participants
This change will deduplicate enum and input definitions by placing them in a global types file (
${outputPath}/globalTypes.ts). Those global types will no longer be printed into the "local" type files, instead they will be imported.To not trigger
noUnusedLocalserrors, the actual used global types are determined bygetGlobalTypesUsedForOperation. This is the part I see the most risk of a regression within this PR (the rest is straightforward). It could be that imports are either under- or over- added. I did test this with a live project though and it came out clean.