Skip to content
This repository has been archived by the owner on Apr 3, 2023. It is now read-only.

refactor: elaborate connector error messages #135

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

darcyYe
Copy link
Contributor

@darcyYe darcyYe commented Mar 17, 2023

Summary

elaborate connector error messages

Testing

UTs.

Checklist

  • .changset-staged
  • unit tests
  • integration tests
  • docs

OR

  • This PR is not applicable for the checklist

@darcyYe darcyYe requested a review from wangsijie March 17, 2023 03:14
@darcyYe darcyYe requested a review from gao-sun as a code owner March 17, 2023 03:14
@linear
Copy link

linear bot commented Mar 17, 2023

LOG-4881 Connector expose as much error info as possible

Do not opt out detailed information

@darcyYe darcyYe force-pushed the yemq-log-4881-expose-connector-error-details branch from bab33b7 to dfdf63c Compare March 17, 2023 03:14
.changeset/lazy-kiwis-explain.md Outdated Show resolved Hide resolved
packages/connector-alipay-web/src/index.ts Outdated Show resolved Hide resolved
packages/connector-alipay-web/src/index.ts Outdated Show resolved Hide resolved
packages/connector-aliyun-dm/src/index.ts Outdated Show resolved Hide resolved
packages/connector-aliyun-sms/src/index.ts Outdated Show resolved Hide resolved
@@ -148,7 +150,7 @@ describe('Facebook connector', () => {
set: jest.fn(),
get: jest.fn(),
})
).rejects.toMatchError(new ConnectorError(ConnectorErrorCodes.SocialAccessTokenInvalid));
).rejects.toMatchError(new ConnectorError(ConnectorErrorCodes.General, JSON.stringify('')));
Copy link
Member

Choose a reason for hiding this comment

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

why do we need manually stringify here?

packages/connector-facebook/src/index.ts Outdated Show resolved Hide resolved
packages/connector-github/src/index.test.ts Outdated Show resolved Hide resolved
@@ -126,7 +126,7 @@ describe('getUserInfo', () => {
const connector = await createConnector({ getConfig });
await expect(
connector.getUserInfo({ code: 'code' }, jest.fn(), { set: jest.fn(), get: jest.fn() })
).rejects.toMatchError(new ConnectorError(ConnectorErrorCodes.SocialAccessTokenInvalid));
).rejects.toMatchError(new ConnectorError(ConnectorErrorCodes.General, JSON.stringify('')));
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines 83 to 86
assert(
accessToken,
new ConnectorError(ConnectorErrorCodes.SocialAuthCodeInvalid, 'accessToken is missing.')
);
Copy link
Member

Choose a reason for hiding this comment

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

actually the access token logic is very repetitive as far as i can see. should be extracted into a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this check is necessary for almost all social connectors. We can extract to @logto/connector-kit and seems it is not a blocker of GA.

Copy link
Member

Choose a reason for hiding this comment

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

created issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check LOG-5808

@darcyYe darcyYe force-pushed the yemq-log-4881-expose-connector-error-details branch 3 times, most recently from 300b075 to ef9f1c4 Compare March 20, 2023 03:37
@darcyYe darcyYe force-pushed the yemq-log-4881-expose-connector-error-details branch from ef9f1c4 to 241ed48 Compare March 20, 2023 03:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants