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.

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

feat(api-graphql): custom subscriptions #13154

Merged
merged 4 commits into from
Mar 21, 2024
Merged

feat(api-graphql): custom subscriptions #13154

merged 4 commits into from
Mar 21, 2024

Conversation

iartemiev
Copy link
Member

@iartemiev iartemiev commented Mar 21, 2024

Draft until this is merged: aws-amplify/amplify-data#131

TODO:

  • Update data-schema & data-schema-types dep versions after merging ^

Description of changes

Adds custom subscriptions support to data-client.

Given this schema

const schema = a.schema({
...,
onPostLiked: a
  .subscription()
  .for(a.ref('likePostReturnPost'))
  .returns(a.ref('Post'))
  .handler(a.handler.custom({ entry: './jsResolver_base.js' })),

onPostUpdated: a
  .subscription()
  .for(a.ref('Post').mutations(['update']))
  .arguments({ postId: a.string() })
  .returns(a.ref('Post'))
  .handler(a.handler.custom({ entry: './jsResolver_base.js' })),

}).authorization([a.allow.private()]),

data-client makes the following subscription functions available:

const sub = client.subscriptions.onPostLiked().subscribe({ next: ..., error: ... })
const sub2 = client.subscriptions.onPostUpdated({ postId: 'abc123' }).subscribe({ next: ..., error: ... })

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@iartemiev iartemiev marked this pull request as ready for review March 21, 2024 15:02
@iartemiev iartemiev requested review from a team as code owners March 21, 2024 15:02
@@ -1,7 +1,6 @@
{
"editor.detectIndentation": false,
"editor.insertSpaces": false,
"editor.tabSize": 4,
Copy link
Member Author

Choose a reason for hiding this comment

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

The workspace settings defined in this file override local VS Code settings, so this effectively forces everyone to use tabSize: 4. Removing it so contributors can use their tab size of choice.

svidgen
svidgen previously approved these changes Mar 21, 2024
Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

No blockers jumping out at me.

Comment on lines 112 to 142
const options = args[args.length - 1] as CustomOperationOptions;

let contextSpec: AmplifyServer.ContextSpec | undefined;
let arg: QueryArgs | undefined;

if (useContext) {
contextSpec = args[0] as AmplifyServer.ContextSpec;
if (contextSpec?.token === undefined) {
throw new Error(
`Invalid first argument passed to ${operation.name}. Expected contextSpec`,
);
}
}

if (argsDefined) {
if (useContext) {
arg = args[1] as QueryArgs;
} else {
arg = args[0] as QueryArgs;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I can't brain it right now, but it would be slick if there were a way to ingest these variations with some type guarding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. contextSpec and options have a predictable shape, so we can guard those.

Copy link
Member Author

Choose a reason for hiding this comment

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

(will update shortly)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a type guard for contextSpec, but options and args are actually not deterministically distinguishable via type guard, because options consists of only optional params and we can't be sure that args won't contain an argument with the same name. Updated with a slight improvement, but not fully type guarded.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Could a type guard be applied to ...args as a whole with guarding based on length and the shape of the first arg?

(Not a blocker.)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting idea. I think it can work. Mind if I play around with that as a follow up?

Comment on lines +86 to +88
.secondaryIndexes(index => [
index('title'),
index('description').sortKeys(['viewCount']),
Copy link
Member Author

@iartemiev iartemiev Mar 21, 2024

Choose a reason for hiding this comment

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

Lines up the DX with aws-amplify/amplify-data#132

@@ -120,7 +120,7 @@ const schema = a.schema({
argumentContent: a.string().required(),
})
.returns(a.ref('EchoResult'))
.function('echoFunction')
.handler(a.handler.function('echoFunction'))
Copy link
Member Author

Choose a reason for hiding this comment

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

Migrating these to the new DX as well. We still have tests in data-schema that ensure the now-deprecated top-level .function functionality is intact.

@iartemiev iartemiev force-pushed the custom-subscriptions branch from 6936c55 to b14e138 Compare March 21, 2024 20:51
@iartemiev iartemiev merged commit 2b71e29 into main Mar 21, 2024
31 checks passed
@iartemiev iartemiev deleted the custom-subscriptions branch March 21, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants