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

chore(utils): Update internal service update input validation #8265

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

adrien2p
Copy link
Member

What
Add more validation to the update input arguments to catch wrong usage

FIXES TRI-64

Copy link

vercel bot commented Jul 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 25, 2024 8:25am
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Jul 25, 2024 8:25am
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2024 8:25am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2024 8:25am
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2024 8:25am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2024 8:25am
resources-docs ⬜️ Ignored (Inspect) Visit Preview Jul 25, 2024 8:25am

Copy link

changeset-bot bot commented Jul 24, 2024

⚠️ No Changeset found

Latest commit: 2f5be74

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@adrien2p adrien2p marked this pull request as ready for review July 24, 2024 10:32
@adrien2p adrien2p requested a review from a team as a code owner July 24, 2024 10:32
@adrien2p
Copy link
Member Author

I am also trying to improve the base API in a separate pr @olivermrbl, though, I ll have to ts expect error in many services because they override the base method with a different API which is not supposed to happen, but I can ignore them for now until the full modules API is settled and in any case those override normalize their input to be consumed by the base methods

@@ -252,12 +252,22 @@ export function MedusaInternalService<
input: any | any[] | SelectorAndData | SelectorAndData[],
@MedusaContext() sharedContext: Context = {}
): Promise<InferEntityType<TEntity> | InferEntityType<TEntity>[]> {
if (!isDefined(input) || (Array.isArray(input) && input.length === 0)) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use isPresent here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes you are right 👍 much better, I ll add that in

@@ -252,12 +252,22 @@ export function MedusaInternalService<
input: any | any[] | SelectorAndData | SelectorAndData[],
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: @shahednasser, do we have a reference for the internal module services? If not, would it make sense to have such that we can provide a link in the error message?

Copy link
Member

Choose a reason for hiding this comment

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

Not a reference, but we have examples here: https://docs.medusajs.com/v2/advanced-development/modules/service-factory#generated-methods

We can generate a reference if we find it necessary, but the reason I didn't is I imagine the typings being confusing. We can alternatively have a document that provides more advanced / realistic examples

@adrien2p
Copy link
Member Author

adrien2p commented Jul 25, 2024

@olivermrbl I will stop this pr for now as it is still documented. But it require a much broader work on aligning the different API's the modules are defining VS what the internal service is provided. There is too much discrepancies for now and I d like to finalise my other tasks. wdyt?

thought, I ll keep this pr open for reference in case I get back to this one later

@olivermrbl
Copy link
Contributor

@olivermrbl I will stop this pr for now as it is still documented. But it require a much broader work on aligning the different API's the modules are defining VS what the internal service is provided. There is too much discrepancies for now and I d like to finalise my other tasks. wdyt?

thought, I ll keep this pr open for reference in case I get back to this one later

Yeah, let's do that for now. We can pick it up as part of our polishing phase.

@olivermrbl olivermrbl marked this pull request as draft July 25, 2024 08:30
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants