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 a0dde86 on JeffML:subscribe into 167681c 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 ***@***.***> 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 <#121 (comment)>, 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.

[danielrearden]

Sorry for the delay. This looks like a good start. I left a few comments. Feel free to leave additional replies to any comment, or ping me on discourse if you think a longer discussion is warranted

[JeffML]

All test seem to be passing, and I've run prettier on the code before pushing to my branch. To my understanding, this automatically updates the pull request.

Coveralls is still showing a fractional drop in coverage (-0.03%), but I can't seem to find the link that shows me what source is not covered. Anyhow, if you can re-review, great. I can go forward with the other databases, or we might talk first about how to DRY up some of the testing.

[danielrearden]

Left additional comments.

[danielrearden]

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.

[danielrearden]

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

[danielrearden]

suggestion: This test can be removed.

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

[danielrearden]

suggestion: This comment is no longer necessary.

[danielrearden]

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.

[danielrearden]

question: Why are we deleting a customer here?

[danielrearden]

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

[danielrearden]

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.

[danielrearden]

see previous comment

[danielrearden]

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

Click on the "CHANGED" tab to view changed files

Files with less coverage are shown in red

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

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.

[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 ***@***.***> wrote: ***@***.**** commented on this pull request. Left additional comments. ------------------------------ In .gitignore <#121 (comment)> : > @@ -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 <#121 (comment)> : > @@ -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 <#121 (comment)> : > @@ -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 <#121 (comment)> : > + + 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 <#121 (comment)> : > + 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 <#121 (comment)>, 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 <#121 (comment)> : > + + 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 <#121 (comment)> : > + 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 <#121 (comment)> : > @@ -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 <#121 (comment)> : > @@ -74,6 +74,7 @@ export type Association = { export type BuilderOptions = { knex: Knex dialect: Dialect + pubSub?: any see previous comment ------------------------------ In src/queryBuilder/createOne.ts <#121 (comment)> : > @@ -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 <#121 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2VY3J4V7DXC77J76LIKGTRZ7ET7ANCNFSM4OA4YFSQ> .