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

feat(medusa): Expose an activeManager_ getter in TransactionBaseService #3256

Merged
merged 12 commits into from
Feb 16, 2023

Conversation

adrien2p
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2023

🦋 Changeset detected

Latest commit: 3761221

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

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

@adrien2p adrien2p force-pushed the feat/active-manager branch 2 times, most recently from c46ef9c to 4b58554 Compare February 14, 2023 10:02
@adrien2p adrien2p marked this pull request as ready for review February 14, 2023 10:47
@adrien2p adrien2p requested a review from a team as a code owner February 14, 2023 10:47
Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues left a comment

Choose a reason for hiding this comment

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

LGTM. Nice cleanup

packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/sales-channel-inventory.ts Outdated Show resolved Hide resolved
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.

Such a nice clean up! @srindom can I get you to sign off on this too :)

@olivermrbl
Copy link
Contributor

olivermrbl commented Feb 16, 2023

@adrien2p Should be updated to use the new Typeorm repository API

@adrien2p
Copy link
Member Author

@adrien2p Should be updated to use the new Typeorm repository API

@olivermrbl as soon as develop will be merged into it and I resolve the conflict it will use the new repo 💪

@olivermrbl
Copy link
Contributor

as soon as develop will be merged into it

Yep sorry, that was what I meant 😅

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.

Super nice! The only thing that I can think of as a potential risk with this is if people have done:

// my-custom-service.ts

import { TransactionBaseService } from "@medusajs/medusa"

class MyService extends TransactionBaseService {
 ...
}

TS would complain after an update right? So this is a breaking change that should be highlighted.

@adrien2p
Copy link
Member Author

adrien2p commented Feb 16, 2023

Super nice! The only thing that I can think of as a potential risk with this is if people have done:

// my-custom-service.ts

import { TransactionBaseService } from "@medusajs/medusa"

class MyService extends TransactionBaseService {
 ...
}

TS would complain after an update right? So this is a breaking change that should be highlighted.

I am not sure what would TS complain about? everything that was done previously can still live, we just add a new helper getter

@srindom
Copy link
Collaborator

srindom commented Feb 16, 2023

Was unsure if the change from protected abstract -> protected would make the compiler complain, but if it doesn't then all good 💪

@adrien2p
Copy link
Member Author

adrien2p commented Feb 16, 2023

Was unsure if the change from protected abstract -> protected would make the compiler complain, but if it doesn't then all good 💪

@srindom it does not complain because now it is just optional to define it on the child class, but if it is present it is not a problem since protected members are accessible from the child

@olivermrbl olivermrbl changed the title feat(medusa): Update transaction base service to expose an activeManager_ getter feat(medusa): Expose an activeManager_ getter in TransactionBaseService Feb 16, 2023
@kodiakhq kodiakhq bot merged commit aefe5aa into develop Feb 16, 2023
@kodiakhq kodiakhq bot deleted the feat/active-manager branch February 16, 2023 10:26
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