-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix(medusa): Correct MedusaRequest query type #14100
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
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
olivermrbl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat
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 |
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