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(): Autoload module resources #7291

Merged
merged 26 commits into from
May 13, 2024

Conversation

adrien2p
Copy link
Member

@adrien2p adrien2p commented May 10, 2024

What

  • automatically build and consume connection and container loader if not exported by the module
  • therefore load the services and repositories automatically, including baseRepository
  • automatically build run and revert migrations if not provided
  • cleaup modules to remove extra unnecessary bits and pieces
  • remove the initializeFactory in favor of using medusaApp

Should drastically improve the module building DX by removing a lot of boilerplate to handle by the user, that plus the base entity should simplify quite a lot the flow cc @shahednasser

Note
I had to choose a way to identify connection and container loader from the exported loader from the module. I decided to go with named function connectionLoader and containerLoader, also, now the factories will return named function so if the user use the factories we are providing to build those loaders, the function will also be named and identified

Copy link

vercel bot commented May 10, 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 💬 Add feedback May 13, 2024 10:52am
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 13, 2024 10:52am
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
docs-ui ⬜️ Ignored (Inspect) Visit Preview May 13, 2024 10:52am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview May 13, 2024 10:52am

Copy link

changeset-bot bot commented May 10, 2024

⚠️ No Changeset found

Latest commit: aa9aa3b

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
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.

At first glance, this is an incredible improvement – nice work!!

Imo, we can push this through, just want to keep it open for others to glance over it too

)

if (!hasConnectionLoader && models.length > 0) {
const connectionLoader = ModulesSdkUtils.mikroOrmConnectionLoaderFactory({
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to get rid of MikroORM here eventually

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the default connection loader, since we are using mikro orm by default. Could you elaborate on your idea?

packages/modules/auth/src/index.ts Outdated Show resolved Hide resolved
@olivermrbl
Copy link
Contributor

Next up is the module service boilerplate 😉

@adrien2p
Copy link
Member Author

Next up is the module service boilerplate 😉

Yes, it will be the last one, then only the custom loaders will remain to export if you have any. This is because we need to keep the order in which they are exported as it might have an impact. Maybe we can think of something there too 👍

@adrien2p
Copy link
Member Author

@olivermrbl should be good to re review if you want 👍

@olivermrbl
Copy link
Contributor

Will do!

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

@kodiakhq kodiakhq bot merged commit 6362342 into develop May 13, 2024
18 checks passed
@kodiakhq kodiakhq bot deleted the chore/module-loading-improvements-dx branch May 13, 2024 12:12
@adrien2p
Copy link
Member Author

@shahednasser just for info it is merged now

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.

None yet

2 participants