Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ node_modules
*.iml
*.code-workspace
.DS_Store
sakila.db
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: sakila.db is created and modified while running the SQLite integration tests. We don't want to check it by accident, so this should be left as-is.


9 changes: 9 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"scripts": {
"build": "rimraf dist && tsc",
"test": "jest --runInBand --coverage",
"testOnly": "jest --runInBand",
"coveralls": "cat ./coverage/lcov.info | coveralls",
"lint": "eslint src",
"format": "prettier --write \"src/**/*.ts\"",
Expand Down Expand Up @@ -79,6 +80,7 @@
"graphql": "15.1.0",
"graphql-middleware": "4.0.2",
"graphql-scalars": "1.1.5",
"graphql-subscriptions": "^1.1.0",
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: This should be in dependencies since it will be used during runtime and not just during build time

"graphql-tag": "2.10.3",
"husky": "4.2.5",
"jest": "26.0.1",
Expand All @@ -95,4 +97,4 @@
"peerDependencies": {
"graphql": "^14.0.0 || ^15.0.0"
}
}
}
3 changes: 2 additions & 1 deletion src/__tests__/postgres/client.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createSqlmancerClient } from '../../'
import { SqlmancerClient } from './sqlmancer'
import { PubSub } from 'graphql-subscriptions'

export const client = createSqlmancerClient<SqlmancerClient>(__dirname + '/schema.ts', require('./knex'))
export const client = createSqlmancerClient<SqlmancerClient>(__dirname + '/schema.ts', require('./knex'), new PubSub())
62 changes: 60 additions & 2 deletions src/__tests__/postgres/integration.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable jest/require-top-level-describe */
/* eslint-disable no-useless-escape */
import { graphql, validateSchema } from 'graphql'
import { schema, client } from './schema'
import { graphql, validateSchema, subscribe, parse, ExecutionResult } from 'graphql'
import { schema, client, pubSub } from './schema'

const describeMaybeSkip = process.env.DB && !process.env.DB.split(' ').includes('postgres') ? describe.skip : describe

Expand Down Expand Up @@ -394,4 +394,62 @@ describeMaybeSkip('integration (postgres)', () => {
expect(errors).toBeUndefined()
expect(data?.films.some((f: any) => f.sequel && f.sequel.id)).toBe(true)
})

test('subscriptions', async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: This test can be removed.

This test is extraneous because it's not testing anything specific to the library.

const document = parse(`
subscription {
create
}
`)

const sub = <AsyncIterator<ExecutionResult>>await subscribe(schema, document)
expect(sub.next).toBeDefined()

const next = sub.next() // grab the promise
pubSub.publish('CREATE_ONE', { create: 'FLUM!' }) //publish
const {
value: { errors, data },
} = await next // await the promise

expect(errors).toBeUndefined()
expect(data).toBeDefined()
expect(data.create).toBe('FLUM!')
})

// this skipped test is returning "GraphQLError: Cannot return null for non-nullable field Subscription.create" at `expect(subErrors).toBeUndefined()`
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: This comment is no longer necessary.

test('subscription triggered by mutation', async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: This probably needs to be converted to a unit test for now.

Like I said before, we should probably start with unit tests to directly exercise any methods that are being added. The unit tests include a harness that lets you roll back any changes when doing mutations so that the tests don't have to worry about side-effects. We should probably add something similar to the integration tests. Right now I don't have any mutations included in the integration tests for this reason.

const document = parse(`
subscription {
create
}
`)

const query = `mutation {
deleteCustomer(id: 1009)
Copy link
Owner

Choose a reason for hiding this comment

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

question: Why are we deleting a customer here?


createCustomer (input: {
Copy link
Owner

Choose a reason for hiding this comment

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

praise: Thanks for modifying this to exercise the publish method.

id: 1009
firstName: "Morty"
lastName: "Blinkers"
email: "[email protected]"
}) {
id
}
}`

const sub = <AsyncIterator<ExecutionResult>>await subscribe(schema, document)
expect(sub.next).toBeDefined()

const next = sub.next()

const { data, errors } = await graphql(schema, query)
expect(errors).toBeUndefined()
expect(data).toBeDefined()

const {
value: { errors: subErrors, data: subData },
} = await next
expect(subErrors).toBeUndefined()
expect(subData).toBeDefined()
}, 10000)
})
20 changes: 18 additions & 2 deletions src/__tests__/postgres/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,17 @@ import { IResolvers } from 'graphql-tools'
import knex from './knex'
import { createSqlmancerClient, makeSqlmancerSchema } from '../..'
import { SqlmancerClient } from './sqlmancer'
import { PubSub } from 'graphql-subscriptions'

const typeDefs = gql`
scalar DateTime
scalar JSON
scalar JSONObject

type Subscription {
create: String!
}

type Query
@sqlmancer(
dialect: POSTGRES
Expand Down Expand Up @@ -243,14 +248,24 @@ const typeDefs = gql`
}
`

export const client = createSqlmancerClient<SqlmancerClient>(__filename, knex)
const pubSub = new PubSub()
export const client = createSqlmancerClient<SqlmancerClient>(__filename, knex, pubSub)

const { Film, Actor, Customer, Address, Movie, Person } = client.models

const resolvers: IResolvers = {
DateTime: GraphQLDateTime,
JSON: GraphQLJSON,
JSONObject: GraphQLJSONObject,

Subscription: {
create: {
subscribe: () => {
return pubSub.asyncIterator('CREATE_ONE')
},
},
},

Query: {
actors: (_root, _args, _ctx, info) => {
return Actor.findMany().resolveInfo(info).execute()
Expand Down Expand Up @@ -291,7 +306,7 @@ const resolvers: IResolvers = {
},
Mutation: {
createCustomer: async (_root, args, _ctx, info) => {
const id = await Customer.createOne(args.input).execute()
const id = await Customer.createOne(args.input).publish().execute()
return Customer.findById(id).resolveInfo(info).execute()
},
createCustomers: async (_root, args, _ctx, info) => {
Expand Down Expand Up @@ -339,3 +354,4 @@ export const schema = makeSqlmancerSchema({
typeDefs,
resolvers,
})
export { pubSub }
5 changes: 3 additions & 2 deletions src/client/createSqlmancerClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ export type GenericSqlmancerClient = Knex & {

export function createSqlmancerClient<T extends GenericSqlmancerClient = GenericSqlmancerClient>(
glob: string,
knex: Knex
knex: Knex,
pubSub?: any
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
pubSub?: any
import { PubSub } from 'graphql-subscriptions'
pubSub: PubSub

In TypeScript, classes represent both types and values. PubSub describes both the type of an instance of PubSub and the underlying ES class.

): T {
const typeDefs = getTypeDefsFromGlob(glob)

Expand All @@ -51,7 +52,7 @@ export function createSqlmancerClient<T extends GenericSqlmancerClient = Generic

return Object.assign(knex, {
models: _.mapValues(models, (model: Model) => {
const options = { knex, dialect }
const options = { knex, dialect, pubSub }
const { builders, readOnly } = model
return {
findById: (id: ID) => new builders.findById(options, id),
Expand Down
10 changes: 10 additions & 0 deletions src/queryBuilder/createOne.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ export class CreateOneBuilder<TCreateFields extends Record<string, any>> extends
this._data = data
}

/**
* Publishes and event notifying subscriber of Change
*/
public publish(): this {
if (this._options.pubSub) {
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: Add tests to cover both branches of this if statement

You can view test coverage issues by clicking on the "Details" link under the failing Coveralls check

Screen Shot 2020-07-04 at 8 33 50 PM

Click on the "CHANGED" tab to view changed files

Screen Shot 2020-07-04 at 8 34 11 PM

Files with less coverage are shown in red

Screen Shot 2020-07-04 at 8 34 21 PM

Click the file link for a detailed view with problem areas highlighted

Screen Shot 2020-07-04 at 8 34 41 PM

The current test only exercises this method when this._options.pubSub is truthy. We need to also account for scenarios where that's not the case. This is also something that's better handled through unit tests than the integration tests.

this._options.pubSub.publish('CREATE_ONE', { create: 'TEST' })
}
return this
}

/**
* Executes the query and returns a Promise that will resolve to the ID of the created row.
*/
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export type Association = {
export type BuilderOptions = {
knex: Knex
dialect: Dialect
pubSub?: any
Copy link
Owner

Choose a reason for hiding this comment

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

see previous comment

}

export type QueryBuilderContext = {
Expand Down