Skip to content

Conversation

@hirsch88
Copy link
Member

No description provided.

@hirsch88 hirsch88 self-assigned this Dec 10, 2017
@hirsch88 hirsch88 requested a review from dweber019 December 10, 2017 16:55
};

public async run(root: any, args: CreatePetMutationArguments, context: GraphQLContext<any, any>): Promise<Pet> {
const petService = context.container.get<PetService>(PetService);
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have this over DI, but think for the next step.

} else {
/* tslint:disable */
// @ts-ignore
orderedResult.push(null);
Copy link
Member

Choose a reason for hiding this comment

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

Could we use undefined as any as T?

Copy link
Member Author

Choose a reason for hiding this comment

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

must be null is given from the lib

};

const allFiles = directories.reduce((allDirs, dir) => {
return allDirs.concat(require('glob').sync(path.normalize(dir)));
Copy link
Member

Choose a reason for hiding this comment

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

It there a reason why we don't import glob at the top?

let connection: Connection;
beforeAll(async () => connection = await createDatabaseConnection());
beforeEach(() => synchronizeDatabase(connection));
afterAll(() => closeDatabase(connection));
Copy link
Member

Choose a reason for hiding this comment

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

Even though the closeDatabase function didn't exist before my review commits it has not thrown a error in TS.
Based on this fact any TS errors in integration tests, which don't fail the app hard, will not be detected by the developer.

};

export const synchronizeDatabase = (connection: Connection) => {
return connection.synchronize(true);
Copy link
Member

Choose a reason for hiding this comment

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

We should use migrations as sync is not aligned with migrations and migrations is the truth representation of the schema.

@hirsch88 hirsch88 merged commit c4c269c into develop Dec 11, 2017
@hirsch88 hirsch88 deleted the feature/graphql branch December 18, 2017 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants