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

Parse saleor schema version and add it to context #339

Merged
merged 5 commits into from
Feb 21, 2024

Conversation

krzysztofzuraw
Copy link
Member

@krzysztofzuraw krzysztofzuraw commented Feb 20, 2024

Parse the saleor-schema-version header and include the parsed version in the contexts of createHandler and webhookFactory. If the header is absent (Saleor version below 3.15), the version will default to 0.

Connected RFC.

Copy link

changeset-bot bot commented Feb 20, 2024

🦋 Changeset detected

Latest commit: 6e59049

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 Minor

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

@krzysztofzuraw krzysztofzuraw changed the title Parse saleor schema version Parse saleor schema version and add it to context Feb 20, 2024
@krzysztofzuraw krzysztofzuraw marked this pull request as ready for review February 20, 2024 12:35
import { AppManifest } from "../../types";

export type CreateManifestHandlerOptions = {
manifestFactory(context: {
appBaseUrl: string;
request: NextApiRequest;
schemaVersion: number;
Copy link
Member

Choose a reason for hiding this comment

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

I dont think it should fallback to zero. I think it should be number | null. If field is not supported, then its not supported. No need for ambiguous values

@@ -149,4 +150,27 @@ describe("processAsyncSaleorWebhook", () => {
})
).rejects.toThrow("Request signature check failed");
});

it("Fallback to 0 if saleor-schema-version header is missing", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

please add test for parsing correct value

import { AppManifest } from "../../types";

export type CreateManifestHandlerOptions = {
manifestFactory(context: {
appBaseUrl: string;
request: NextApiRequest;
schemaVersion: number | null;
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking; you can add comment (jsdoc) mentioning here that null means <3.15

IDEs are using comments for hints

Copy link
Contributor

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

Install it with

pnpm add @saleor/[email protected]

Copy link
Contributor

Package Line Rate Branch Rate Complexity Health
src 79% 95% 0
src.APL 85% 85% 0
src.APL.saleor-cloud 75% 86% 0
src.APL.vercel-kv 54% 69% 0
src.app-bridge 88% 86% 0
src.handlers.next 90% 85% 0
src.handlers.next.saleor-webhooks 94% 81% 0
src.middleware 60% 97% 0
src.settings-manager 96% 89% 0
src.test-utils 100% 90% 0
src.util 76% 100% 0
Summary 83% (3795 / 4560) 87% (447 / 514) 0

@krzysztofzuraw krzysztofzuraw merged commit 492bae2 into main Feb 21, 2024
5 checks passed
@krzysztofzuraw krzysztofzuraw deleted the add-schema-version branch February 21, 2024 09:01
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