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: remove container registration name #9137

Merged
merged 13 commits into from
Sep 16, 2024

Conversation

carlos-r-l-rodrigues
Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues commented Sep 15, 2024

What:

  • Deprecated ModuleRegistrationName in favor of Modules enum
  • Modules are always registered in the container using their key
  • Modules' key name normalized to PascalCase and Service suffix was removed (breaking change if module is resolved using strings)

Copy link

vercel bot commented Sep 15, 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 Sep 16, 2024 7:27am
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Sep 16, 2024 7:27am
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2024 7:27am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2024 7:27am
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2024 7:27am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2024 7:27am
resources-docs ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2024 7:27am

Copy link

changeset-bot bot commented Sep 15, 2024

⚠️ No Changeset found

Latest commit: 8bd4b40

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

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, we might need a release note for that :D

Copy link
Member

@sradevski sradevski left a comment

Choose a reason for hiding this comment

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

Really nice cleanup! Q: Are we also cleaning up the ContainerRegistrationKeys?

@carlos-r-l-rodrigues
Copy link
Contributor Author

Really nice cleanup! Q: Are we also cleaning up the ContainerRegistrationKeys?

Yeah, I think we need. I haven't done in this PR because I dont have any good naming suggestion.

@carlos-r-l-rodrigues
Copy link
Contributor Author

@olivermrbl, let me know if we can merge this one.

@olivermrbl
Copy link
Contributor

Yes, all good

@carlos-r-l-rodrigues carlos-r-l-rodrigues merged commit 950cf9a into develop Sep 16, 2024
23 checks passed
@olivermrbl
Copy link
Contributor

Maybe ContainerRegistrationKeys could be split into something like Resources and Tools:

const query = container.resolve(Tools.QUERY)
const logger = container.resolve(Tools.LOGGER)
const linker = container.resolve(Tools.REMOTE_LINK) // might want to rename this at some point?

const pgConnection = container.resolve(Resources.PG_CONNECTION)
const config = container.resolve(Resources.CONFIG)

Hard to find a single term that encapsulates all, because of how different these all are.

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