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

Mark query in manifest required #280

Merged
merged 2 commits into from
Sep 6, 2023
Merged

Mark query in manifest required #280

merged 2 commits into from
Sep 6, 2023

Conversation

lkostrowski
Copy link
Member

@lkostrowski lkostrowski commented Sep 4, 2023

In AppManifest, webhook query field is not required. Saleor rejects empty query, so Manifest was wrong before with with field optional

This is technically a breaking change, but app without a query specified couldn't be installed in Saleor.

Now Typescript will early show the error

@lkostrowski lkostrowski requested a review from a team September 4, 2023 07:44
@changeset-bot
Copy link

changeset-bot bot commented Sep 4, 2023

🦋 Changeset detected

Latest commit: f7dedce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@saleor/app-sdk Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

Released snapshot build with @dev tag in npm with version: 0.0.0-pr-20230904074518

Install it with

pnpm add @saleor/[email protected]

@lkostrowski
Copy link
Member Author

verify if notify webhook works

@lkostrowski
Copy link
Member Author

@krzysztofwolski I checked E&M and it looks fine

export const notifyWebhook = new SaleorAsyncWebhook<NotifySubscriptionPayload>({
  name: "notify",
  webhookPath: "api/webhooks/notify",
  asyncEvent: "NOTIFY_USER",
  apl: saleorApp.apl,
  query: "{}", // We are using the default payload instead of subscription
});

it defines query as string which is ok for the new SDK

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Released snapshot build with @dev tag in npm with version: 0.0.0-pr-20230905173654

Install it with

pnpm add @saleor/[email protected]

Comment on lines 5 to 9
In AppManifest, webhook query field is not required. Saleor rejects empty query, so Manifest was wrong before with with field optional

This is technically a breaking change, but app without a query specified couldn't be installed in Saleor.

Now Typescript will early show the error
Copy link
Member

@peelar peelar Sep 6, 2023

Choose a reason for hiding this comment

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

It doesn't explicitly say what the change is

Copy link
Member Author

Choose a reason for hiding this comment

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

rewrtitten

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

Released snapshot build with @dev tag in npm with version: 0.0.0-pr-20230906071152

Install it with

pnpm add @saleor/[email protected]

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

Package Line Rate Branch Rate Complexity Health
src 83% 95% 0
src.APL 80% 85% 0
src.app-bridge 88% 86% 0
src.handlers.next 90% 84% 0
src.handlers.next.saleor-webhooks 93% 82% 0
src.middleware 60% 97% 0
src.settings-manager 96% 89% 0
src.test-utils 100% 90% 0
src.util 76% 100% 0
Summary 85% (3286 / 3878) 87% (398 / 455) 0

@lkostrowski lkostrowski merged commit 3e31b3a into main Sep 6, 2023
4 checks passed
@lkostrowski lkostrowski deleted the update-manifest-query branch September 6, 2023 07:30
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.

2 participants