fix(medusa): Correct MedusaRequest query type#14100
fix(medusa): Correct MedusaRequest query type#14100NicolasGorga wants to merge 8 commits intodevelopfrom
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 17. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
🦋 Changeset detectedLatest commit: 39aeddf The changes in this PR will be included in the next version bump. This PR includes changesets to release 74 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
olivermrbl
left a comment
There was a problem hiding this comment.
pretty sure we have done this all over the place; are you sure this is correct? if so, maybe we need to update more places
I'm positive. Included fixes to all the incorrect usages. |
adrien2p
left a comment
There was a problem hiding this comment.
I think it is fine, I am just wondering if it would make sense to do something like this
export interface AuthenticatedMedusaRequest<
BodyOrQueryFields = unknown,
QueryFields extends Record<string, unknown> = {}
> extends MedusaRequest<
{} extends QueryFields ? undefined : BodyOrQueryFields,
{} extends QueryFields
? BodyOrQueryFields & Record<string, unknown>
: QueryFields
> {
auth_context: AuthContext
publishable_key_context?: PublishableKeyContext
}export interface MedusaRequest<
Body = unknown,
QueryFields extends Record<string, unknown> = {}
> extends Request<
{ [key: string]: string },
any,
{} extends QueryFields ? Body : undefined
> {
validatedBody: {} extends QueryFields ? undefined : Body
validatedQuery: {} extends QueryFields
? RequestQueryFields
: RequestQueryFields & QueryFields
//...Here we would allow for
- single template args -> query fields
- both -> body, fields
Let me know if that makes sense guys (cc @olivermrbl)
I like this since I think users might not consistently understand that they should put |
|
Hey @adrien2p I've updated the types with your suggestion, with a small correction on the Also, if you could help me know how we could handle the following error when building in src/http/utils/validate-body.ts:31:7 - error TS2322: Type 'ZodRawShape' is not assignable to type 'undefined'.
31 req.validatedBody = await zodValidator(schema, req.body)
~~~~~~~~~~~~~~~~~
Found 1 error.This happens when not providing any types. QueryFields is the default value, which treats the Body param as the query one and evaluates to undefined. |
didn't see the discussion, I think adrien's suggestion makes sense to incorporate if its feasible
|
Yes, added the changes to the type as per Adrien suggestion, though that introduces the issue I mentioned in my last comment when we don't pass the types and use defaults, while try to assign to validatedBody, since in these cases it is treated as undefined (as per the "if only one type provided, we assume no body and only query"). I will wait for his input :) |
Yeah I ve suggeted undefined but it can be a record or unknown or something like that, the main idea is to route the correct template args to the correct location 👍 |
|
I am still having build issues, thinking of how to solve this with the changes we did |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
Thank you for working on this! After reviewing this PR, there are a few things that need to be addressed before we can move forward: Required changes:
Concern: |
Summary
What — What changes are introduced in this PR?
Updates
MedusaRequestgenerics in /store/currencies get endpoints.Why — Why are these changes relevant or necessary?
To align the body and query types to what is actually expected.
How — How have these changes been implemented?
Corrected
MedusaRequestgenerics adding undefined as the first type parameter so the already passed type is correctly mapped toqueryinstead ofbody.Testing — How have these changes been tested, or how can the reviewer test the feature?
Examples
Provide examples or code snippets that demonstrate how this feature works, or how it can be used in practice.
This helps with documentation and ensures maintainers can quickly understand and verify the change.
// Example usageChecklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
Add any additional context, related issues, or references that might help the reviewer understand this PR.
closes ENTSUP-279