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(data): events client #13923

Merged
merged 12 commits into from
Oct 23, 2024
Merged

feat(data): events client #13923

merged 12 commits into from
Oct 23, 2024

Conversation

iartemiev
Copy link
Member

Description of changes

Adds events client

Description of how you validated changes

  1. Manual testing w sample app
  2. E2E tests pass - https://github.com/aws-amplify/amplify-js/actions/runs/11348758601
  3. Unit tests added/pass

Checklist

Checklist for repo maintainers

  • Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows

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

Copy link

⚠️ This PR includes changes to the "aws-amplify" package.json file, which can have library-wide implications.

Please ensure that this PR:

  • Does not manually change "@aws-amplify/*" dependency versions, which may misalign core dependencies across the library
  • Remove any export paths without a major version bump

A repository administrator is required to review this change.

@@ -1181,7 +1181,7 @@ describe('AWSAppSyncRealTimeProvider', () => {
});

test('authenticating with AWS_LAMBDA/custom w/ custom header function that accepts request options', async () => {
expect.assertions(2);
expect.assertions(3);
Copy link
Member Author

Choose a reason for hiding this comment

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

This test asserts on the number of times the additionalHeaders function gets executed. Since we split out WS connect into its own method, additionalHeaders gets evaluated one more time in this test, but resolves to the same value. It's a no-op in terms of behavior.

@@ -677,7 +675,9 @@ describe('AWSAppSyncRealTimeProvider', () => {
}),
);

expect(socketCloseSpy).toHaveBeenNthCalledWith(1, 3001);
await delay(1);
Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing an occasional race condition here without the tick delay. Test is unchanged otherwise.

};
}

export abstract class AWSWebSocketProvider {
Copy link
Member Author

Choose a reason for hiding this comment

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

This class is functionally identical to AWSAppSyncRealTimeProvider in main. Methods that deal with AppSync vs Events specifics were converted to abstract and each respective provider implements those. Some of the larger methods were split up for legibility as well.

@iartemiev
Copy link
Member Author

⚠️ This PR includes changes to the "aws-amplify" package.json file, which can have library-wide implications.

Please ensure that this PR:

* Does not manually change "@aws-amplify/*" dependency versions, which may misalign core dependencies across the library

* Remove any export paths without a major version bump

A repository administrator is required to review this change.

Note: these are just expected bundle size value updates. No dependencies or versions were changed in this PR.

@@ -12,6 +12,7 @@ import {
import { ConnectionState as CS } from '../src/types/PubSub';

import { AWSAppSyncRealTimeProvider } from '../src/Providers/AWSAppSyncRealTimeProvider';
import { isCustomDomain } from '../src/Providers/AWSWebSocketProvider/appsyncUrl';
Copy link
Member Author

Choose a reason for hiding this comment

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

isCustomDomain used to be a private method on AWSAppSyncRealTimeProvider but we moved it out into a util.

svidgen
svidgen previously approved these changes Oct 18, 2024
@@ -223,6 +224,28 @@ function parseData(
};
}

function parseCustom(
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we add unit tests for this new parse function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. It's tested indirectly as part of this describe block: https://github.com/aws-amplify/amplify-js/pull/13923/files#diff-d114e2967bd860edbf0a234243cc226fe3f6215597d5392dc7b4c276b6e8eaafR47

But I'll follow up with a couple more in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iartemiev iartemiev merged commit 847fb51 into main Oct 23, 2024
30 checks passed
@iartemiev iartemiev deleted the feat/events/main branch October 23, 2024 18:37
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.

3 participants