Skip to content

Conversation

@kddejong
Copy link

@kddejong kddejong commented May 11, 2022

Description of changes

AppSync now supports unsubscribing connections . Previously this message is not available to uses of the amplify-js libraries. This PR will allow a user to capture this completion message and take the appropriate action inside their application

Issue #, if available

Description of how you validated changes

const subscription = API.graphql<Observable<{
      provider: any;
      value: GraphQLResult<any>;
    }>>(graphqlOperation(onCreateMessage))
    .subscribe({
      next: ({ provider, value }: {provider: any, value: GraphQLResult}) => {
        console.log({ provider, value });
      },
      complete: () => console.log('Complete'),
      error: ({ provider, error }: {provider: any, error: GraphQLResult}) => {
        error.errors?.forEach(e => {
          console.log(e);
        });
      }
    });

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.

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.47%. Comparing base (9831d3d) to head (86b72d9).
⚠️ Report is 3023 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9898      +/-   ##
==========================================
+ Coverage   78.34%   83.47%   +5.12%     
==========================================
  Files         250      253       +3     
  Lines       18339    18201     -138     
  Branches     3955     3901      -54     
==========================================
+ Hits        14368    15193     +825     
+ Misses       3840     2918     -922     
+ Partials      131       90      -41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@siegerts siegerts added the first-time-contributor The contribution is the first for this user in the repo label May 12, 2022
@kddejong
Copy link
Author

Added tests and now have good codecov delta

Copy link
Contributor

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

This change looks like it will work as you propose. It also appears to add functionality that wouldn't impact the existing behavior. Have you linked it to a real project and exercised the behavior this change plugs into?

I think its technically an API change and it would be good to have verification that its working as expected in an end to end use case.

If you've done that validation and remove the comment, this change has my approval.

start: console.error,
next: value => observer.next({ provider, value }),
error: error => observer.error({ provider, error }),
// complete: observer.complete, // TODO: when all completed, complete the outer one
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove this comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor first-time-contributor The contribution is the first for this user in the repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Amplify PubSub unsubscribe method does not report the status/complete before sign out?

4 participants