Dynamic modules serialize symbol providers as strings

[svvac]

The fix to #5964 introduced in bacef33 seems incomplete. Because the provided token gets serialized as a string, using provide: Symbol() gets serialized as "Symbol()" which leads to the same collapse as in the initial issue.

By design, Symbol() !== Symbol() so treating them as strings defeats the purpose and leads to the same behavior as initially described in #5964.

Using the reproduction code from the original issue and by adapting TestModule.testProviderSymbol() as such :

     public static testProviderSymbol (feature: any): Provider {
-        const provide = Symbol(String(feature))
+        const provide = Symbol();
         return {
             provide,
             inject: [ 'test' ],
             useFactory: (secret: any) => console.log('feature', feature, provide),
         }
     }

The providers 2 and 3 disappear from the module :

[Nest] 51298   - 01/07/2021, 12:40:39 PM   [NestFactory] Starting Nest application...
feature 1 Symbol()
feature 4 6797326
feature 5 15789009
feature 6 15349412
feature 7 Symbol()
feature 7 16697338
feature 8 Symbol()
feature 8 7560964
feature 9 Symbol()
feature 9 775357
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +17ms
feature 10 Symbol()
feature 11 Symbol()
feature 12 Symbol()
feature 13 4502118
feature 14 485346
feature 15 5842275
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +1ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +0ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +0ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +0ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +0ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +1ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] TestModule dependencies initialized +0ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [InstanceLoader] AppModule dependencies initialized +0ms
[Nest] 51298   - 01/07/2021, 12:40:39 PM   [NestApplication] Nest application successfully started +5ms

Versions

$ yarn list @nestjs/\*
├─ @nestjs/[email protected]
├─ @nestjs/[email protected]
└─ @nestjs/[email protected]

[Tony133]

Hi @svvac, I recommend that you provide a minimal reproduction of the problem in a git repo that is clonable so the core team can better verify this.

[svvac]

Alright I'll piece it together :

@Module({})
export class TestModule {
    public static forRoot (): DynamicModule {
        return {
            module: TestModule,
            global: true,
            providers: [ { provide: 'test', useValue: { secret: 42 } } ],
            exports: [ 'test' ]
        };
    }

    public static forFeatureRandom (feature: any): DynamicModule {
        return {
            module: TestModule,
            providers: [ this.testProviderRandom(feature) ],
        };
    }

    public static forFeatureSymbol (feature: any): DynamicModule {
        return {
            module: TestModule,
            providers: [ this.testProviderSymbol(feature) ],
        };
    }

    public static forFeatureBoth (feature: any): DynamicModule {
        return {
            module: TestModule,
            providers: [ this.testProviderSymbol(feature), this.testProviderRandom(feature) ],
        };
    }


    public static testProviderRandom (feature: any): Provider {
        const provide = String(Math.random() * 0xffffff | 0);
        return {
            provide,
            inject: [ 'test' ],
            useFactory: (secret: any) => console.log('feature', feature, provide),
        }
    }

    public static testProviderSymbol (feature: any): Provider {
        const provide = Symbol();
        return {
            provide,
            inject: [ 'test' ],
            useFactory: (secret: any) => console.log('feature', feature, provide),
        }
    }
}

@Module({
    imports: [
        TestModule.forRoot(),
        TestModule.forFeatureSymbol(1),
        TestModule.forFeatureSymbol(2),
        TestModule.forFeatureSymbol(3),
        TestModule.forFeatureRandom(4),
        TestModule.forFeatureRandom(5),
        TestModule.forFeatureRandom(6),
        TestModule.forFeatureBoth(7),
        TestModule.forFeatureBoth(8),
        TestModule.forFeatureBoth(9),
    ],
    providers: [
        TestModule.testProviderSymbol(10),
        TestModule.testProviderSymbol(11),
        TestModule.testProviderSymbol(12),
        TestModule.testProviderRandom(13),
        TestModule.testProviderRandom(14),
        TestModule.testProviderRandom(15),
    ],
})
export class AppModule { }

[kamilmysliwiec]

There's one, very important characteristic of Nest's dynamic modules. Basically, every time you import a dynamic module (anywhere in your application) with the exact same metadata (the configuration you pass to the dynamic module), Nest will eventually create a single node in the graph (a singleton) instead of 2 nodes with the same configuration (it's an optimization technique allowing you to call, for example, ModuleX.forFeature() multiple times in multiple places with the same arguments, and still, have just a single module representation in the memory). Every time you create a dynamic module, we generate a unique hash identifier which allows us to easily compare whether dynamic modules registered within your application are equal (and if so, "optimise" them).

Now for this, we must serialise the metadata (and so symbols) in order to safely generate a hash. In theory, we could track occurrences of symbols (in dynamic metadata records) while seralising objects, and storing used symbols in a globally defined map to make sure we always use a proper uuid for each symbol. The problem here is that we cannot use a WeakMap for this (issue tc39/ecma262#1194, more on that topic here tc39/ecma262#1194 (comment)), at least until this tc39/ecma262#2038 is accepted and implemented in JS. Therefore, we'd have to use, for example, a Map collection instead which in turn may unnecessarily increase the memory consumption and in tests, may potentially lead to memory leaks.

Can you please explain why you want to use Symbol() in your module's metadata in the first place? I'm thinking about potential use cases for that and I can't find one.

[svvac]

Thanks for the technical details.

I'm trying to (ab)use dynamic modules for a modular configuration system where each nest module can register configuration loaders and schemas, and I need a painless way to ensure that two modules can't override each other's stuff. Also I'd like the configuration to be loaded as soon as possible while instantiating the module tree so that other modules can easily make use of the config to initialize themselves.

I don't actually need them injected anywhere, just to execute during the initialization phase to populate various services (It's actually kind of a hack to support « multi providers » ; #770, #4786).

I can't use object/class refs because of #5591 ; random strings feel dirty and leave a chance of collision (albeit small) ; so symbols seemed like the best fit for my use case.

---

Despite certainly being useful, the module deduplication feels like a footgun to me. First I can't remember seeing any mention of that in the docs. Also, the example above shows that providing a factory with a different function with a different local scope and a different token leads to an unexpected collapse.

Perhaps there could be an escape hatch where a dynamic module can provide its own deduplication id that bypasses the « naive » string hashing done by default ?

---

To me there is a « hole » in Nest's DI system where despite using modules to namespace various application components, I consistently end up hitting some kind of global namespace where I need to somehow coordinate wildly different modules to not step on each other's toes. This issue feels a lot like #5591 where function/class identity gets collapsed into Function.name across the dependency tree. Here symbols are treated as « second-class citizens » and despite their use being recommended throughout the docs they don't provide anything more than a simple string would. It is even more frustrating that test cases 4-15 all work as expected with no obvious (to me, and probably to anyone not familiar with Nest's internals) reason

[kamilmysliwiec]

To me there is a « hole » in Nest's DI system where despite using modules to namespace various application components, I consistently end up hitting some kind of global namespace

I'm not sure how this entire issue relates to the DI system. You mentioned that the only way you want is "to execute during the initialization phase to populate various services (It's actually kind of a hack to support « multi providers » ; #770, #4786)" which in fact, is a hack and isn't something that DI system was supposed to address in the first place. If you want to do something like this, generating a unique ID as a provider just to allow Nest to somehow distinguish metadata objects sounds completely fine.

Here symbols are treated as « second-class citizens » and despite their use being recommended throughout the docs they don't provide anything more than a simple string would.

This isn't true. The only way symbols are - sort of treated as "second-class citizens" - is while serializing dynamic metadata and only for the sake of generating a hash. This doesn't change their actual behavior in the DI context at all and doesn't make them comparable to strings (behavior-wise). While resolving providers & injecting services/classes that use symbols as tokens, Nest will use the actual symbol references (not their stringified representation). Likewise, it will use symbol references to determine what's exported from the module (if there are any providers that use symbols as tokens), not their stringified versions.

First I can't remember seeing any mention of that in the docs.

I do agree we should mention that in the docs (very likely in this chapter https://docs.nestjs.com/fundamentals/dynamic-modules or as a hint here https://docs.nestjs.com/modules). If you want to contribute, PRs are more than welcome (https://docs.nestjs.com/modules). Currently, there might not be any mention of that (I'm not sure tbh) but this sounds likely as you're the first person who actually asked about this over the past 4 years 😅 And as we have limited time too, we typically prioritize things based on developers (framework consumers) interest.

Anyways, for now, there's no plan to change the current behavior, especially that it might introduce a breaking change in many NestJS applications (since this is how it works from the beginning). I'll think what are the pros and cons of potentially dropping this optimization and just making all dynamic modules unique ootb regardless of their configuration, but since it's a pretty huge change, it definitely requires somewhat more time to think.