nest

nest copied to clipboard

Is there an existing issue that is already proposing this?

• [X] I have searched the existing issues

Is your feature request related to a problem? Please describe it

Not really a problem, just makes things easier if you use as const

Describe the solution you'd like

I would like these types to allow readonly arrays.

Teachability, documentation, adoption, migration strategy

I don't think this is necessary

What is the motivation / use case for changing the behavior?

I'm working on a module which should have some of the same providers for forRoot and forRootAsync, to get these I call a function that returns a readonly array. I could just not use a readonly array, but I don't see it as a bad thing to utilize the typescript type system, so I don't accidentally do something weird with the array that was returned.

@Module({})
export class MindsdbModule {
  public static forRoot(options?: MindsdbModuleOptions): DynamicModule {
    const mindsdbModuleOptions = {
      provide: MINDSDB_MODULE_OPTIONS,
      useValue: options,
    };

    const providers = [
      ...this.getBaseProviders(),
      mindsdbModuleOptions,
    ];
    return {
      global: true,
      module: MindsdbModule,
      imports: [ScheduleModule.forRoot()],
      providers,
      exports: [MindsdbService, RetrainJobService],
    };
  }

  private static getBaseProviders() {
    return [
      MindsdbService,
      RetrainJobService,
    ] as const;
  }

  private static getBaseExports() {
    return [
      MindsdbService,
      RetrainJobService,
    ] as const;
  }

  public static forRootAsync(options: MindsdbModuleAsyncOptions): DynamicModule {
    const providers = [
      {
        provide: MINDSDB_MODULE_OPTIONS,
        useFactory: options.useFactory,
        inject: options.inject || [],
      },
      ...this.getBaseProviders(),
      ...(options.extraProviders || []),
    ];

    return {
      module: MindsdbModule,
      imports: options.imports,
      providers,
      exports: this.getBaseExports(),
    };
  }
}

Mar 15 '24 15:03 Alexnortung

I guess that's just a matter of changing the following file

https://github.com/nestjs/nest/blob/e331fb0d996b4fea11bd3ef8e76ef46e8b53b4d1/packages/common/interfaces/modules/module-metadata.interface.ts#L14

Mar 15 '24 15:03 micalevisk

Yes, I think so too

Mar 15 '24 15:03 Alexnortung

I didn't manage to change that without introducing a breaking changing at type level

type AnArray<T> = Array<T> | ReadonlyArray<T>;

/**
 * Interface defining the property object that describes the module.
 *
 * @see [Modules](https://docs.nestjs.com/modules)
 *
 * @publicApi
 */
export interface ModuleMetadata {
  /**
   * Optional list of imported modules that export the providers which are
   * required in this module.
   */
  imports?: AnArray<
    Type<any> | DynamicModule | Promise<DynamicModule> | ForwardReference
  >;
  /**
   * Optional list of controllers defined in this module which have to be
   * instantiated.
   */
  controllers?: AnArray<Type<any>>;
  /**
   * Optional list of providers that will be instantiated by the Nest injector
   * and that may be shared at least across this module.
   */
  providers?: AnArray<Provider>;
  /**
   * Optional list of the subset of providers that are provided by this module
   * and should be available in other modules which import this module.
   */
  exports?: AnArray<
    | DynamicModule
    | Promise<DynamicModule>
    | string
    | symbol
    | Provider
    | ForwardReference
    | Abstract<any>
    | Function
  >;
}

Mar 17 '24 16:03 micalevisk

I really don't think that we can introduce this change without dropping support for external references to those arrays usage such as: ModuleMetadata['imports']

Jun 26 '24 01:06 micalevisk

I'm all for this!

It's unfortunate the pervasive use of this type. It would be much better to refactor it out.

export type ModuleRef = Type<any> | DynamicModule | Promise<DynamicModule> | ForwardReference;

export interface ModuleMetadata {
  imports?: ModuleRef[];
}

This allows easy access to the actual item type, and wrapping that in an array where needed is easy. Where as now it is much more annoying to unwrap:

export type ModuleRef = (ModuleMetadata['imports'] & {}) extends Array<infer U> ? U : never;

I feel it's worth noting that Array satisfies ReadonlyArray, so anywhere that type is used as a function argument would go unchanged.

const colors: string[] = ['red'];
const doThing = (items: readonly string[]) => {};
doThing(colors); // fine

So really it's only the case where you want to .push to an existing module shape's imports that would be breaking. The metadata property isn't readonly so this would be an easy migration for those rare cases push is being used.

module.imports = module.imports?.concat(x)
// or
module.imports = [...module.imports, x]

I did look through the list referenced above and there were only a couple places where .push was used.

Jul 23 '24 03:07 CarsonF