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

fix(module-sdk): Shared modules loading #4611

Merged
merged 6 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/nasty-files-type.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@medusajs/modules-sdk": patch
---

fix(module-sdk): Shared modules loading should not share the entire core container but only the resources that are meant to be shared
8 changes: 1 addition & 7 deletions integration-tests/plugins/medusa-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,8 @@ module.exports = {
},
productModuleService: {
scope: "internal",
resources: "isolated",
resources: "shared",
resolve: "@medusajs/product",
options: {
database: {
clientUrl: DB_URL,
debug: false,
},
},
},
},
}
16 changes: 8 additions & 8 deletions packages/modules-sdk/src/loaders/__tests__/module-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe("modules loader", () => {
container = createMedusaContainer()
})

it("registers service as undefined in container when no resolution path is given", async () => {
it("should register the service as undefined in the container when no resolution path is given", async () => {
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
resolutionPath: false,
Expand Down Expand Up @@ -52,7 +52,7 @@ describe("modules loader", () => {
expect(testService).toBe(undefined)
})

it("registers service ", async () => {
it("should register the service ", async () => {
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
resolutionPath: "@modules/default",
Expand Down Expand Up @@ -93,7 +93,7 @@ describe("modules loader", () => {
expect(typeof testService).toEqual("object")
})

it("runs defined loaders and logs error", async () => {
it("should run the defined loaders and logs the errors if something fails", async () => {
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
resolutionPath: "@modules/brokenloader",
Expand Down Expand Up @@ -121,7 +121,7 @@ describe("modules loader", () => {
)
})

it("logs error if no service is defined", async () => {
it("should log the errors if no service is defined", async () => {
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
resolutionPath: "@modules/no-service",
Expand Down Expand Up @@ -149,7 +149,7 @@ describe("modules loader", () => {
)
})

it("throws error if no service is defined and module is required", async () => {
it("should throw an error if no service is defined and the module is required", async () => {
expect.assertions(1)
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
Expand Down Expand Up @@ -181,7 +181,7 @@ describe("modules loader", () => {
}
})

it("throws error if default package isn't found and module is required", async () => {
it("should throw an error if the default package isn't found and the module is required", async () => {
expect.assertions(1)
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
Expand Down Expand Up @@ -213,7 +213,7 @@ describe("modules loader", () => {
}
})

it("throws error if no scope is defined to the module", async () => {
it("should throw an error if no scope is defined on the module declaration", async () => {
expect.assertions(1)
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
Expand Down Expand Up @@ -245,7 +245,7 @@ describe("modules loader", () => {
}
})

it("throws error if resources is not set when scope is defined as internal", async () => {
it("should throw an error if the resources is not set when scope is defined as internal", async () => {
expect.assertions(1)
const moduleResolutions: Record<string, ModuleResolution> = {
testService: {
Expand Down
28 changes: 12 additions & 16 deletions packages/modules-sdk/src/loaders/utils/load-internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,20 @@ export async function loadInternalModule(
}
}

const localContainer =
resources === MODULE_RESOURCE_TYPE.ISOLATED
? createMedusaContainer()
: (container.createScope() as MedusaContainer)
const localContainer = createMedusaContainer()

if (resources === MODULE_RESOURCE_TYPE.ISOLATED) {
const moduleDependencies = resolution?.dependencies ?? []
const dependencies = resolution?.dependencies ?? []
if (resources === MODULE_RESOURCE_TYPE.SHARED) {
dependencies.push("manager", "configModule")
}

for (const dependency of moduleDependencies) {
localContainer.register(
dependency,
asFunction(() => {
return container.hasRegistration(dependency)
? container.resolve(dependency)
: undefined
})
)
}
for (const dependency of dependencies) {
localContainer.register(
dependency,
asFunction(() => {
return container.resolve(dependency, { allowUnregistered: true })
})
)
}

const moduleLoaders = loadedModule?.loaders ?? []
Expand Down
11 changes: 9 additions & 2 deletions packages/product/src/loaders/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { EntitySchema } from "@mikro-orm/core"

import * as ProductModels from "@models"
import { createConnection } from "../utils"
import { ModulesSdkTypes } from "@medusajs/types"
import { ConfigModule, ModulesSdkTypes } from "@medusajs/types"

export default async (
{
Expand All @@ -28,7 +28,14 @@ export default async (
moduleDeclaration?.scope === MODULE_SCOPE.INTERNAL &&
moduleDeclaration.resources === MODULE_RESOURCE_TYPE.SHARED
) {
return
const { projectConfig } = container.resolve("configModule") as ConfigModule
options = {
database: {
clientUrl: projectConfig.database_url!,
driverOptions: projectConfig.database_extra!,
schema: projectConfig.database_schema!,
},
}
}

const customManager = (
Expand Down