Subscribe

[JeffML]

subscribe impl notes

• added graphql-subscriptions as a dev dependency
• added optional pubsub arg to createSqlMancerClient(); type of any (TODO: typedef)
• added pubsub to options arg that is passed to resolvers
• added pubsub to BuilderOptions type
• in tests/postgres/schema.ts I added Subscription type and resolver
  • imported PubSub from graphql-subscriptions; called by resolver
• created a test (thanks for your help!) in integration.test.js.
• you'll notice I'm exporting a pubsub instance from schema.js and using that for the tests

Other miscellaneous stuff

• I added another test that attempts to use a trigger an event by using an actual mutation, but it doesn't work. It might be something to do with the mock but I didn't get too deep into it.
• added a testOnly script in package.json that omits the --coverage flag
  • I used env DB=postgres POSTGRES_PASSWORD=testpassword npm run testOnly integration to run the tests
• added sakila.db to .gitignore, because it was an empty file

---

I would call this a proof-of-concept implementation, nothing more. I'm sure there are improvements.

Remaining work:

1. write a test that goes through the client to create a customer and trigger publish()
2. add an argument to publish to pass as subscription payload
3. write implementations for CreateMany, DeleteById, DeleteMany, UpdateById, and UpdateMany
4. implement for other databases

---

Question: Why the heck is setimmediate required in the test? I think this has something to do with lazy initialization of the AsyncIterator returned by graphql-subscriber publish(), but my thinking is pretty fuzzy on how setImmediate resolves that.

[coveralls]

Coverage decreased (-0.03%) to 90.791% when pulling a0dde86d5e55082fc97a5b82ca0c0b2d1dae5dbb on JeffML:subscribe into 167681c20bca4233e5bdbd9e1738e35da5f3438b on danielrearden:develop.

[danielrearden]

@JeffML Thanks for all your work on this. I'm sorry I still haven't had a chance to review it -- will do so as soon as possible

[JeffML]

No worries.

On Thu, Jun 18, 2020 at 6:03 AM Daniel Rearden [email protected] wrote:

@JeffML https://github.com/JeffML Thanks for all your work on this. I'm sorry I still haven't had a chance to review it -- will do so as soon as possible

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/danielrearden/sqlmancer/pull/121#issuecomment-646002885, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2VY3P74N55EFB6FBKTYJ3RXIGCNANCNFSM4OA4YFSQ .

[JeffML]

I think the reduce coverage is due to my not implementing for other databases yet, so they are not getting the extra pubsub parameter.

[JeffML]

Ok, thanks for the detailed feedback. I appreciate that you're busy and trying not to make too much work for you

On Sat, Jul 4, 2020, 5:48 PM Daniel Rearden [email protected] wrote:

@danielrearden commented on this pull request.

Left additional comments.

In .gitignore https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449815688 :

@@ -6,4 +6,4 @@ node_modules

*.iml

*.code-workspace

.DS_Store

-sakila.db

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.

In package.json https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449815751 :

@@ -79,6 +80,7 @@

 "graphql": "15.1.0",

 "graphql-middleware": "4.0.2",

 "graphql-scalars": "1.1.5",

• "graphql-subscriptions": "^1.1.0",

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

In src/tests/postgres/integration.test.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449816309 :

@@ -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 () => {

suggestion: This test can be removed.

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

In src/tests/postgres/integration.test.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449816318 :

• 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()

suggestion: This comment is no longer necessary.

In src/tests/postgres/integration.test.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449816415 :

• 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()

• test('subscription triggered by mutation', async () => {

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

Like I said before https://github.com/danielrearden/sqlmancer/pull/121#discussion_r446514166, 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.

In src/tests/postgres/integration.test.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449816441 :

• 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()

• test('subscription triggered by mutation', async () => {

• const document = parse(`

•  subscription {
  

•    create
  

•  }
  

• `)

• const query = `mutation {

•  deleteCustomer(id: 1009)
  

question: Why are we deleting a customer here?

In src/tests/postgres/integration.test.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449816494 :

• 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()

• test('subscription triggered by mutation', async () => {

• const document = parse(`

•  subscription {
  

•    create
  

•  }
  

• `)

• const query = `mutation {

•  deleteCustomer(id: 1009)
  

•  createCustomer (input: {
  

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

In src/client/createSqlmancerClient.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449817101 :

@@ -38,7 +38,8 @@ export type GenericSqlmancerClient = Knex & {

export function createSqlmancerClient<T extends GenericSqlmancerClient = GenericSqlmancerClient>(

glob: string,

• knex: Knex

• knex: Knex,

• pubSub?: any

⬇️ Suggested change

• pubSub?: any

+import { PubSub } from 'graphql-subscriptions'

+pubSub: PubSub

In TypeScript, classes represent both types and values https://www.typescriptlang.org/docs/handbook/classes.html#constructor-functions. PubSub describes both the type of an instance of PubSub and the underlying ES class.

In src/types.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449817143 :

@@ -74,6 +74,7 @@ export type Association = {

export type BuilderOptions = {

knex: Knex

dialect: Dialect

• pubSub?: any

see previous comment

In src/queryBuilder/createOne.ts https://github.com/danielrearden/sqlmancer/pull/121#discussion_r449817926 :

@@ -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) {

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

[image: Screen Shot 2020-07-04 at 8 33 50 PM] https://user-images.githubusercontent.com/18018864/86523215-1ac7c580-be37-11ea-91a6-a31f512637b1.png

Click on the "CHANGED" tab to view changed files

[image: Screen Shot 2020-07-04 at 8 34 11 PM] https://user-images.githubusercontent.com/18018864/86523217-1f8c7980-be37-11ea-82b8-e209eaa69df5.png

Files with less coverage are shown in red

[image: Screen Shot 2020-07-04 at 8 34 21 PM] https://user-images.githubusercontent.com/18018864/86523219-23200080-be37-11ea-815e-d59f6e8a1914.png

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

[image: Screen Shot 2020-07-04 at 8 34 41 PM] https://user-images.githubusercontent.com/18018864/86523222-261af100-be37-11ea-9d92-0dd479b8f64c.png

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/danielrearden/sqlmancer/pull/121#pullrequestreview-442637037, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2VY3J4V7DXC77J76LIKGTRZ7ET7ANCNFSM4OA4YFSQ .