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.

Sign up for GitHub

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

Jump to bottom

feat(medusa, stock-location, inventory): Allow modules to integrate with core #2997

Merged
merged 21 commits into from
Jan 13, 2023
Merged

feat(medusa, stock-location, inventory): Allow modules to integrate with core #2997

merged 21 commits into from
Jan 13, 2023

Conversation

carlos-r-l-rodrigues
Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues commented Jan 11, 2023 •
edited
Loading

What:
Enhanced Module's configs enabling them to share the Core's EntityManager.
Modules will be able to run integrated in the Core and have the option to use their own resources like Database, or use the same as the Core.

FIXES: CORE-980

@changeset-bot
Copy link

changeset-bot bot commented Jan 11, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 1e12b8b

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

This PR includes changesets to release 3 packages
Name Type
@medusajs/inventory Patch
@medusajs/medusa Patch
@medusajs/stock-location 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

@carlos-r-l-rodrigues carlos-r-l-rodrigues changed the base branch from master to develop January 11, 2023 16:52
@carlos-r-l-rodrigues carlos-r-l-rodrigues marked this pull request as ready for review January 11, 2023 17:13
@carlos-r-l-rodrigues carlos-r-l-rodrigues requested a review from a team as a code owner January 11, 2023 17:13
srindom
srindom reviewed Jan 11, 2023
View reviewed changes
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.

Think this is great - feels like a good first approach. Would like to understand how we will approach the TO as well.

packages/inventory/src/index.ts Show resolved Hide resolved
packages/inventory/src/loaders/connection.ts Outdated Show resolved Hide resolved
packages/inventory/src/services/inventory-item.ts Show resolved Hide resolved
adrien2p
adrien2p requested changes Jan 12, 2023
View reviewed changes
packages/inventory/src/services/inventory.ts Outdated Show resolved Hide resolved
packages/inventory/src/services/inventory.ts Show resolved Hide resolved
packages/medusa/src/loaders/module-definitions/index.ts Show resolved Hide resolved
packages/medusa/src/loaders/module.ts Outdated Show resolved Hide resolved
packages/medusa/src/types/global.ts Show resolved Hide resolved
srindom
srindom approved these changes Jan 12, 2023
View reviewed changes
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!

@olivermrbl olivermrbl changed the title chore: module shared resources feat(medusa, stock-location, inventory): Allow modules to integrate with core Jan 12, 2023
olivermrbl
olivermrbl approved these changes Jan 13, 2023
View reviewed changes
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.

LGTM, just a single thought :)

packages/stock-location/src/services/stock-location.ts Outdated Show resolved Hide resolved
@olivermrbl
Copy link
Contributor

@adrien2p When you have time, can I get you to run another review, so we get closer to merging?

adrien2p
adrien2p reviewed Jan 13, 2023
View reviewed changes
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM: one question and one point:
I really don't think that we should start putting repository specificities into the service. Having to manage the query builder and relations from the service seams a bit off. I let you look at the comment and let me know what you think.

packages/inventory/src/services/inventory-item.ts Outdated Show resolved Hide resolved
packages/stock-location/src/services/stock-location.ts Show resolved Hide resolved
@olivermrbl
Copy link
Contributor

olivermrbl commented Jan 13, 2023 •
edited
Loading

@adrien2p Please approve if the current state LGTY :)

@adrien2p
Copy link
Member

@carlos-r-l-rodrigues if you are alright with the repository specific work in the service for that pr then you can merge and tackle that later. Just want to avoid accumulating debt across pr as it is easily forgotten. up to you

@carlos-r-l-rodrigues carlos-r-l-rodrigues merged commit 9dbccd9 into develop Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivermrbl olivermrbl olivermrbl approved these changes

@adrien2p adrien2p adrien2p approved these changes

@srindom srindom srindom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@carlos-r-l-rodrigues @olivermrbl @adrien2p @srindom