feat(core): added unique dynamic module factory

[H4ad]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior?

Today, we don't have any way to skip the hashing algorithm of ModuleTokenFactory.

Refs: #12719
Refs: #12753 (comment)

What is the new behavior?

In this new way, we have this new module that we can use to avoid the serialization of ModuleTokenFactory:

class MyDynamicModule {
  static forRoot(customProvider: string, customText: string) {
    return {
      module: MyDynamicModule,
      providers: [
        {
          provide: customProvider,
          useValue: customText,
        },
      ],
      exports: [customProvider],
    } satisfies DynamicModule;
  }
}

@Injectable()
export class AppService {
  constructor(
    @Inject('123') protected readonly myDynamicText1: string,
    @Inject('456') protected readonly myDynamicText2: string,
  ) {}

  getHello(): string {
    return `From Inject: ${this.myDynamicText1} ${this.myDynamicText2}`;
  }
}

@Controller()
export class AppController {
  constructor(private readonly appService: AppService) {}

  @Get()
  getHello(): string {
    return this.appService.getHello();
  }
}

@Module({
  imports: [
    UniqueModuleFactory.wrap('myFirstDynamic', MyDynamicModule.forRoot('123', 'hello')),
    UniqueModuleFactory.wrap('mySecondDynamic', MyDynamicModule.forRoot('456', 'world')),
  ],
  controllers: [AppController],
  providers: [AppService],
})
export class AppModule {}

This also solves the issue on #12719, just adds .wrap to that module.

Why do you need the staticUniqueId?

Because of the following comment: #10844 (comment)

If we didn't have to maintain the same ID, we could drop the staticModuleId.

Does this PR introduce a breaking change?

• Yes
• No

Other information

I opened it as a draft because I want to get some feedback about this API before starting to write tests.

/cc @micalevisk @jmcdo29

[micalevisk]

I'm a bit concerned on this being static

what if we want to create multiple apps (let's say, one NestFactory.createApplicationContext and other with NestFactory.create)

I can't think of any other approach tho

[kamilmysliwiec]

what if we want to create multiple apps (let's say, one NestFactory.createApplicationContext and other with NestFactory.create)

Just chiming in here - I don't think anyone should be doing that 😄

[micalevisk]

yeah, me too :p So we can treat that as a limitation of UniqueDynamicModuleFactory if someone try to do so

[micalevisk]

I'd use | as the separator because _ is valid in class names so one could have this:

and so .split('|', 1)[0]

[micalevisk]

I was thinking of another API for this I believe yours is better

1. UniqueModuleFactory.for('myFirstDynamic', MyDynamicModule.forRoot('123', 'hello'))
2. UniqueModuleFactory.forModule(MyDynamicModule.forRoot('123', 'hello'), 'myFirstDynamic')

---

Another concern of mine: does module re-exporting still works? what about app.select(dynamicModule)? not sure if the are tests enough to cover those usages

[kamilmysliwiec]

We may introduce a breaking change in one of the next major releases instead of complicating the API even further. Upon importing a module, the framework would internally assign a private attribute (symbol?) to the dynamic module object which serves as a "unique, predictable key" of that module. This means that if one does this:

imports: [TypeOrmModule.forFeature([User])],

and elsewhere:

imports: [TypeOrmModule.forFeature([User])],

again then 2 modules would be created. However, if one does this instead:

const MyTypeOrmModule = TypeOrmModule.forFeature([User]);

// and then
imports: [MyTypeOrmModule],

// and somewhere else
imports: [MyTypeOrmModule],

Then only 1 module node would be created.

Now when it comes to generating hashes, they may still come in handy for tools like devtools, and serialized graphs. For this, we could use a simplified algorithm where instead of serializing literally everything we just serialize the module name, its providers (tokens, not values), and possibly exports and imports too (this is something to figure out).

[H4ad]

@kamilmysliwiec I already did that with WeakMap, I describe that approach at #10844 (comment) under Using object reference as an ID. But that approach breaks the serialization across multiple runs.

simplified algorithm where instead of serializing literally everything we serialize the module name, its providers (tokens, not values), and possibly exports and imports too (this is something to figure out).

This will not have a good outcome, trying to decide what is important and what is not. I think it will be better to give the user a feature, and they decide whether they use it or not, instead of trying to guess their use cases.

What we can also do is expose that hash algorithm and let the user change that implementation, and then they decide what they want to ignore during the hashing, instead of leaving that choice to us. This is something that you already mentioned in the past.

But even with the feature of giving the user the freedom to choose how the hash is generated, I think it will not have any harm to also add this feature of skipping the hash entirely.

[kamilmysliwiec]

What we can also do is expose that hash algorithm and let the user change that implementation, and then they decide what they want to ignore during the hashing, instead of leaving that choice to us. This is something that you already mentioned in the past.

99,9% of users wouldn't ever make use of that simply because most framework users don't (and really shouldn't) care about how they (modules) are internally represented & orchestrated.

@kamilmysliwiec I already did that with WeakMap, I describe that approach at #10844 (comment) under Using object reference as an ID. But that approach breaks the serialization across multiple runs.

That's why I proposed generating predictable keys.

This will not have a good outcome, trying to decide what is important and what is not.

What key features of the framework, specifically, rely on predictable serialization?

[micalevisk]

@H4ad

let the user change that implementation

thinking better now, can't we instead expose such feature like this:

// User-land code
class InternalModuleTokenFactory implements Something { /* ... */ }

// before any `NestFactory.create*` call
UniqueDynamicModuleFactory.apply(new InternalModuleTokenFactory())

like we have for durable providers

It would change the behavior of ModuleTokenFactory#create with no hard changes to the user-land code. Not sure if it's feasible, tho

[H4ad]

99,9% of users wouldn't ever make use of that simply because most framework users don't (and really shouldn't) care about how they're internally represented & orchestrated.

I agree with you, most of the time the startup will be very fast, especially if the users don't use dynamic modules. The ones who will use that feature are people who have issues with performance during the startup, who saw the warning that we introduced, and who are interested in improving their initialization time.

This feature that we are discussing is literally for edge-cases, and that's why I think is not good to introduce breaking changes just for those edge-cases.

What key features of the framework, specifically, rely on predictable serialization?

I only know DevTools.

But if we are talking about providers, exports, etc...

The two issues that were opened about slow startup were caused by:

• large/cyclic object
• large buffer

The first one is very hard to avoid/detect.
The second one we maybe can skip but we introduce a breaking change, Is it worth it? I don't think so.

---

@micalevisk I liked your idea, that's a good API to expose the hashing algorithm.

[kamilmysliwiec]

But if we are talking about providers, exports, etc...

The two issues that were opened about slow startup were caused by:

large/cyclic object
large buffer
The first one is very hard to avoid/detect.
The second one we maybe can skip but we introduce a breaking change, Is it worth it? I don't think so.

And this only occurs for providers that use the "useValue" syntax. We don't (and really can't with the current implementation) provide a fully working mechanism that would guarantee uniqueness for providers that use "useFactory" (and there have been plenty of issues reported on this).

The only way to finally fix it is by introducing a breaking change anyway, that's why I'm generally OK with doing that as part of the next major release. If the only reason was performance then yeah perhaps replacing the current solution wouldn't be the most reasonable decision.

PS. "useClass" syntax is generally broken too

[H4ad]

@kamilmysliwiec From what I understand, we have some choices:

• continue with the introduction of the API proposed in this PR
• do what you mentioned about associating a [Symbol] to the object (or use weak map) to avoid serialization.
• some internal refactoring on useValue, useFactory, etc...?

I can help with the first one but I don't have enough knowledge on the other topics to be able to propose a PR, what do you want to do?

[kamilmysliwiec]

Let's track this here #13336