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: Hide repository creation if they are not custom + add upsert support by default #6127

Merged
merged 18 commits into from
Jan 19, 2024

Conversation

adrien2p
Copy link
Member

@adrien2p adrien2p commented Jan 18, 2024

What

  • Create a default container loader consumable by the modules
  • Create and register default base repositories for each model of a module if not provided by the module or the custom repositories options
  • add support for a default upsert implementation (with the current way they are working so without using upsertMany) to the base service and repo
  • cleanup and abstraction

Copy link

changeset-bot bot commented Jan 18, 2024

🦋 Changeset detected

Latest commit: e394e44

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@medusajs/utils Patch
@medusajs/types Patch
@medusajs/product Patch
@medusajs/pricing Patch

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

Copy link

vercel bot commented Jan 18, 2024

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

Name Status Preview Comments Updated (UTC)
api-reference 🔄 Building (Inspect) Visit Preview Jan 19, 2024 1:13pm
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 19, 2024 1:13pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jan 19, 2024 1:13pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jan 19, 2024 1:13pm

@adrien2p
Copy link
Member Author

@srindom here is what it would look like on the product module for the example. We are putting all the repositories that do not have any custom logic into the container directly.

@olivermrbl here you can also have a look to the default upsert implementation. Also, I am wondering why we have done it manually and we haven't used upsertMany. But for now I ve kept it as it is and I ll see later

@adrien2p
Copy link
Member Author

Also seb, here as you can see I ve changed the way the integration tests are loading the service they are trying to test. The same could be done for the unit tests. In that case, we wouldn't need any default constructor for our usage as we would never instanciate them manually. wdyt?

@srindom
Copy link
Collaborator

srindom commented Jan 18, 2024

A few thoughts:

The goal is to minimize the amount of boilerplate needed to setup new data models and start defining business logic around them. Right now, you have to do the following: a) create the model, b) create a repository file where you export a repository, c) ensure that the repository is registered in the container, d) create a service where you use the repository and specify your business logic.

I would argue that b and c could be eliminated altogether for the standard case where you don't want to define anything custom. Instead you simply do: a) create your data model, b) create a service using the abstractServiceFactory. The service can use the associated data model to create the repository on the fly instead of having the repository live in the dependency container.

I think we can even go further with this idea to when you want to customize things in the service layer. You create your data model and create your service with the abstractServiceFactory. The abstract service factory could ensure that there is a repository instance variable on the service class that can be used so I can do

...
myCustomFunction() {
  // this.repository can be used to do operations on the data model passed to abstractServiceFactory
}

If you want to do custom repository methods you can create a repository file, but I would even argue that in that case you should just pass it directly to abstractServiceFactory instead of registering it in the dependency container.

Let me know what you think?

@adrien2p
Copy link
Member Author

adrien2p commented Jan 18, 2024

I see what you mean now @srindom , in the next proposal i was about to extract the loader container logic since most of it is repetitive and can be inferred from the entity with some naming conventions that we already have. Going back to something instanciated from within the service instead of using the DI will make it hard to test. If i want to mock the inner repository then i have to play with lot of mock to reach that goal, it is one of the problem solved by the DI.

In my next proposal, the only thing you have to do is create your service and thats it, to type the repo property you can just play with the RepositoryService interface. If you want a custom repository, you create a new file and thats it, it will be loaded automatically since it follows the naming conventions. Note: it doesn't requires any file reading or anything just to be explicit

@adrien2p
Copy link
Member Author

@srindom @olivermrbl wdyt of this commit does that correspond to what you expected?

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

this is epic <3

Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

LGTM!


const upsertedEntities: T[] = []
const createdEntities: T[] = []
const updatedEntities: T[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

for the upsert we have to return all these 3 as a tuple.
they will be used to emit different events for insert and update.

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, for now I thought keeping it like this to not increase the changes number, but we will still have them available for the changes when we have the event normalisation ready

@adrien2p
Copy link
Member Author

@fPolic @pKorsholm if you can have a look when you are free, I am only missing your view before being able to merge 😄

Copy link
Contributor

@fPolic fPolic left a comment

Choose a reason for hiding this comment

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

Looks good @adrien2p 💪

@olivermrbl
Copy link
Contributor

Merging now – @pKorsholm hope it's OK. @adrien2p can get you up to speed later

@olivermrbl olivermrbl merged commit 5e655dd into develop Jan 19, 2024
16 checks passed
@olivermrbl olivermrbl deleted the chore/module-scaffolding-services-repo-hidden branch January 19, 2024 14:09
@github-actions github-actions bot mentioned this pull request Jan 19, 2024
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.

6 participants