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

Jump to bottom

[TS] Dedup enums and inputs by using global types file #520

Merged
merged 4 commits into from
Aug 2, 2018
Merged

[TS] Dedup enums and inputs by using global types file #520

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9914a0e
[TS] Dedup enums and inputs by using global types file
danilobuerger Jul 31, 2018
eb86363
[TS] Only import global types that are being used
danilobuerger Jul 31, 2018
76b9462
Sync text schema.graphql and .json
danilobuerger Aug 1, 2018
2a9a24b
[TS] Refactored getGlobalTypesUsedForOperation
danilobuerger Aug 1, 2018
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
18 changes: 15 additions & 3 deletions packages/apollo-cli/src/commands/codegen/__tests__/__snapshots__/generate.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,11 @@ exports[`successful codegen writes TypeScript types into a __generated__ directo

export interface SimpleQuery {
hello: string;
}
}"
`;

exports[`successful codegen writes TypeScript types into a __generated__ directory next to sources when no output is set 2`] = `
"

/* tslint:disable */
// This file was automatically generated and should not be edited.
Expand All @@ -618,7 +622,11 @@ exports[`successful codegen writes TypeScript types next to sources when output

export interface SimpleQuery {
hello: string;
}
}"
`;

exports[`successful codegen writes TypeScript types next to sources when output is set to empty string 2`] = `
"

/* tslint:disable */
// This file was automatically generated and should not be edited.
Expand All @@ -644,7 +652,11 @@ exports[`successful codegen writes TypeScript types to a custom directory next t

export interface SimpleQuery {
hello: string;
}
}"
`;

exports[`successful codegen writes TypeScript types to a custom directory next to sources when output is set 2`] = `
"

/* tslint:disable */
// This file was automatically generated and should not be edited.
Expand Down
9 changes: 9 additions & 0 deletions packages/apollo-cli/src/commands/codegen/__tests__/generate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ describe("successful codegen", () => {
expect(
mockFS.readFileSync("directory/__generated__/SimpleQuery.ts").toString()
).toMatchSnapshot();
expect(
mockFS.readFileSync("__generated__/globalTypes.ts").toString()
).toMatchSnapshot();
});

test
Expand Down Expand Up @@ -380,6 +383,9 @@ describe("successful codegen", () => {
.readFileSync("directory/__foo__/SimpleQuery.ts")
.toString()
).toMatchSnapshot();
expect(
mockFS.readFileSync("__foo__/globalTypes.ts").toString()
).toMatchSnapshot();
}
);

Expand Down Expand Up @@ -442,6 +448,9 @@ describe("successful codegen", () => {
.readFileSync("directory/SimpleQuery.ts")
.toString()
).toMatchSnapshot();
expect(
mockFS.readFileSync("globalTypes.ts").toString()
).toMatchSnapshot();
}
);

Expand Down
55 changes: 49 additions & 6 deletions packages/apollo-cli/src/generate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { generateSource as generateSwiftSource } from "apollo-codegen-swift";
import { generateSource as generateTypescriptLegacySource } from "apollo-codegen-typescript-legacy";
import { generateSource as generateFlowLegacySource } from "apollo-codegen-flow-legacy";
import { generateSource as generateFlowSource } from "apollo-codegen-flow";
import { generateSource as generateTypescriptSource } from "apollo-codegen-typescript";
import { generateLocalSource as generateTypescriptLocalSource, generateGlobalSource as generateTypescriptGlobalSource } from "apollo-codegen-typescript";
import { generateSource as generateScalaSource } from "apollo-codegen-scala";
import { GraphQLSchema } from "graphql";
import { FlowCompilerOptions } from '../../apollo-codegen-flow/lib/language';
Expand Down Expand Up @@ -71,12 +71,9 @@ export default function generate(
writeOperationIdsMap(context);
writtenFiles += 1;
}
} else if (target === "flow" || target === "typescript" || target === "ts") {
} else if (target === "flow") {
const context = compileToIR(schema, document, options);
const { generatedFiles, common } =
target === "flow"
? generateFlowSource(context)
: generateTypescriptSource(context);
const { generatedFiles, common } = generateFlowSource(context);

const outFiles: {
[fileName: string]: BasicGeneratedFile;
Expand Down Expand Up @@ -118,6 +115,52 @@ export default function generate(
generatedFiles.map(o => o.content.fileContents).join("\n") + common
);

writtenFiles += 1;
}
} else if (target === "typescript" || target === "ts") {
const context = compileToIR(schema, document, options);
const generatedFiles = generateTypescriptLocalSource(context);
const generatedGlobalFile = generateTypescriptGlobalSource(context);

const outFiles: {
[fileName: string]: BasicGeneratedFile;
} = {};

if (nextToSources || (fs.existsSync(outputPath) && fs.statSync(outputPath).isDirectory())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

if (nextToSources && !fs.existsSync(outputPath)) {
fs.mkdirSync(outputPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was confusing sourcePath and outputPath. You're right, this makes sense for the globalTypes.ts file.

}

const globalSourcePath = path.join(outputPath, "globalTypes.ts");
outFiles[globalSourcePath] = {
output: generatedGlobalFile.fileContents,
};

generatedFiles.forEach(({ sourcePath, fileName, content }) => {
let dir = outputPath;
if (nextToSources) {
dir = path.join(path.dirname(sourcePath), dir);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir);
}
}

const outFilePath = path.join(dir, fileName);
outFiles[outFilePath] = {
output: content({ outputPath: outFilePath, globalSourcePath }).fileContents,
};
});

writeGeneratedFiles(outFiles, path.resolve("."));

writtenFiles += Object.keys(outFiles).length;
} else {
fs.writeFileSync(
outputPath,
generatedFiles.map(o => o.content().fileContents).join("\n") +
generatedGlobalFile.fileContents,
);

writtenFiles += 1;
}
} else {
Expand Down
Loading