-
Notifications
You must be signed in to change notification settings - Fork 31
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: [] add cma adapter to sdk #816
Conversation
Change stale limit to 2 hours instead of 1 day
To prevent some flaky integration tests failing when reaching the rate limit on visiting the entry, this adds a command to visit entries and when failed with a 429 it waits 1second and revisits the page.
@@ -47,6 +48,7 @@ function makeSharedAPI(channel: Channel, data: ConnectMessage): BaseExtensionSDK | |||
const currentLocation = data.location || locations.LOCATION_ENTRY_FIELD | |||
|
|||
return { | |||
cmaAdapter: createAdapter(channel), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were originally planning to just call this adapter
- right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we thought the name adapter
is a little generic and therefore went for cmaAdapter
in case there will be more adapters in the future (e.g. GraphQL adapter?).
Also, the name anyway doesn't match with the cma.js
as it needs to passed in there as apiAdapter
lib/types.ts
Outdated
@@ -705,6 +707,8 @@ export interface BaseExtensionSDK { | |||
access: AccessAPI | |||
/** Exposes relevant ids, keys may be ommited based on location */ | |||
ids: IdsAPI | |||
/** the adapter that can be used for creating the cma client */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this comment is correct, the adapter is already created at the time we create the SDK, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the adapter is created, but the comment says that you can use it to create the cma client, or did I word that wrong?🤔.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is correct but still should be reworded to be consistent with the other comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the commment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I would only (super nit) rephrase the comment as
"Adapter to be injected in contentful-management
client"
this makes clear what repository to look at if you read this thing as a newcomer
# Conflicts: # CHANGELOG.md # lib/types.ts # package-lock.json # package.json
# [4.0.0-alpha.19](v4.0.0-alpha.18...v4.0.0-alpha.19) (2021-07-01) ### Features * [] add cma adapter to sdk ([#816](#816)) ([24dab4e](24dab4e))
🎉 This PR is included in version 4.0.0-alpha.19 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Purpose of PR
PR Checklist