feat: update js-sdk for v48 support#25
Conversation
There was a problem hiding this comment.
Thanks for the PR! I verified locally that the project compiles and builds successfully with no TypeScript errors after installing dependencies.
However, we need to address a few failing checks and do some cleanup before merging:
- Failing Checks:
- Conventional Commits: The PR title and commit message need to follow the conventional commits format (e.g.,
feat: update js-sdk for v48 supportorchore: update js-sdk for v48 support). - Formatting (Prettier): The
pre-commithook failed because many generated files andsrc/extensions.tsare not formatted.
- Conventional Commits: The PR title and commit message need to follow the conventional commits format (e.g.,
- File hygiene: The
temp_projected/directory seems to have been committed accidentally? (we might also want to add it to.gitignoreto prevent this in the future).
I've also left a couple inline comments.
| CheckoutWithFulfillmentResponseSchema, | ||
| CheckoutWithFulfillmentUpdateRequestSchema, | ||
| OrderSchema, | ||
| PaymentClassSchema, |
There was a problem hiding this comment.
A quick AI-assisted review of this code leads me to think you should update normalize-generated-schemas.mjs as we have obsolete mappings and at least one duplication.
There was a problem hiding this comment.
@segiodongo (AI finding) Naming Issue: PaymentClassSchema vs PaymentSchema
In src/spec_generated.ts (and used in src/extensions.ts), we have a schema named PaymentClassSchema (and associated type PaymentClass) representing the split payments configuration (defined in UCP schemas as split_payments.json which extends payment.json).
Why it is named PaymentClassSchema:
- "Class" is a generic suffix appended by
quicktypeto resolve a name collision (see quicktype naming behavior). - The collision occurs because the name
PaymentSchemawas already assigned to the inlinepaymentconfiguration inside the UCP discovery profile generated in scripts/project-current-ucp-schemas.mjs:L613-L621. - You can see the resulting
PaymentSchema(which only containshandlers) at src/spec_generated.ts:L513. - Because the discovery profile's inline property claimed
PaymentSchemafirst, the actual payment instruments schema was bumped toPaymentClassSchema.
Using PaymentClass in the SDK's public API is confusing for consumers. Also, the mapping ["PaymentClass", "PaymentSelection"] in normalize-generated-schemas.mjs is now obsolete because the schemas have structurally diverged in UCP v48 (one has amount, the other has selected).
Proposal:
Since 0.4.0 is a new major release, we should fix this naming issue now to avoid releasing a confusing API name that we will have to break later.
- Remove the obsolete
["PaymentClass", "PaymentSelection"]mapping innormalize-generated-schemas.mjs. - Implement a post-processing renaming rule in
normalize-generated-schemas.mjsto explicitly renamePaymentClasstoPaymentSplitPayments(resulting inPaymentSplitPaymentsSchemaandPaymentSplitPaymentstype). - Update
src/extensions.tsto usePaymentSplitPaymentsSchemainstead ofPaymentClassSchema.
|
added temp_projected/ to git ignore udpated normalize-generate-schema.mjs to remove duplicates also updated formatting to align with prettier checker |
| CheckoutWithFulfillmentResponseSchema, | ||
| CheckoutWithFulfillmentUpdateRequestSchema, | ||
| OrderSchema, | ||
| PaymentClassSchema, |
There was a problem hiding this comment.
@segiodongo (AI finding) Naming Issue: PaymentClassSchema vs PaymentSchema
In src/spec_generated.ts (and used in src/extensions.ts), we have a schema named PaymentClassSchema (and associated type PaymentClass) representing the split payments configuration (defined in UCP schemas as split_payments.json which extends payment.json).
Why it is named PaymentClassSchema:
- "Class" is a generic suffix appended by
quicktypeto resolve a name collision (see quicktype naming behavior). - The collision occurs because the name
PaymentSchemawas already assigned to the inlinepaymentconfiguration inside the UCP discovery profile generated in scripts/project-current-ucp-schemas.mjs:L613-L621. - You can see the resulting
PaymentSchema(which only containshandlers) at src/spec_generated.ts:L513. - Because the discovery profile's inline property claimed
PaymentSchemafirst, the actual payment instruments schema was bumped toPaymentClassSchema.
Using PaymentClass in the SDK's public API is confusing for consumers. Also, the mapping ["PaymentClass", "PaymentSelection"] in normalize-generated-schemas.mjs is now obsolete because the schemas have structurally diverged in UCP v48 (one has amount, the other has selected).
Proposal:
Since 0.4.0 is a new major release, we should fix this naming issue now to avoid releasing a confusing API name that we will have to break later.
- Remove the obsolete
["PaymentClass", "PaymentSelection"]mapping innormalize-generated-schemas.mjs. - Implement a post-processing renaming rule in
normalize-generated-schemas.mjsto explicitly renamePaymentClasstoPaymentSplitPayments(resulting inPaymentSplitPaymentsSchemaandPaymentSplitPaymentstype). - Update
src/extensions.tsto usePaymentSplitPaymentsSchemainstead ofPaymentClassSchema.
Address ptiper comments: - Rename PaymentClass to PaymentSplitPayments - Merge ConsentClassSchema and ConsentValueSchema by resolving aliases before duplicate detection - Remove accidentally committed temp_projected directory - Fix formatting issues TAG=agy CONV=6508c105-0a79-417a-90cd-97ec089641e9
Description
Category (Required)
Please select one or more categories that apply to this change.
ucp-schematool (resolver, linter, validator). (Requires Maintainer approval)Related Issues
Checklist
!for breaking changes).Screenshots / Logs (if applicable)