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: [] add cma adapter to sdk #816

Merged
merged 93 commits into from
Jul 1, 2021
Merged

feat: [] add cma adapter to sdk #816

merged 93 commits into from
Jul 1, 2021

Conversation

loweisz
Copy link
Contributor

@loweisz loweisz commented Jun 28, 2021

Purpose of PR

PR Checklist

  • Tests are added/updated/not required
  • Tests are passing
  • Typescript typings are added/updated/not required

dependabot-preview bot and others added 30 commits May 18, 2021 07:50
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.
@loweisz loweisz marked this pull request as ready for review June 29, 2021 12:56
@loweisz loweisz requested a review from a team as a code owner June 29, 2021 12:56
@loweisz loweisz changed the title feat: add cma adapter to sdk feat: [] add cma adapter to sdk Jun 29, 2021
@@ -47,6 +48,7 @@ function makeSharedAPI(channel: Channel, data: ConnectMessage): BaseExtensionSDK
const currentLocation = data.location || locations.LOCATION_ENTRY_FIELD

return {
cmaAdapter: createAdapter(channel),
Copy link
Contributor

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?

Copy link
Contributor

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 */
Copy link
Contributor

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?

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, the adapter is created, but the comment says that you can use it to create the cma client, or did I word that wrong?🤔.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the commment

Copy link
Contributor

@shikaan shikaan left a 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

@loweisz loweisz changed the base branch from master to canary June 30, 2021 11:21
@loweisz loweisz changed the base branch from canary to master June 30, 2021 11:22
@loweisz loweisz requested a review from Jwhiles June 30, 2021 13:32
@loweisz loweisz changed the base branch from master to canary June 30, 2021 16:14
# Conflicts:
#	CHANGELOG.md
#	lib/types.ts
#	package-lock.json
#	package.json
@loweisz loweisz merged commit 24dab4e into canary Jul 1, 2021
@loweisz loweisz deleted the cma-adapter-call branch July 1, 2021 09:01
ghost pushed a commit that referenced this pull request Jul 1, 2021
# [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))
@ghost
Copy link

ghost commented Jul 1, 2021

🎉 This PR is included in version 4.0.0-alpha.19 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

8 participants