-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Allow readonly arrays as providers, imports, etc. #13326
Comments
I guess that's just a matter of changing the following file
|
Yes, I think so too |
I didn't manage to change that without introducing a breaking changing at type level my attempttype 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
>;
} it's breaking the following line nest/packages/core/injector/container.ts Line 173 in e331fb0
|
Add support for ModuleMetadata to accept ReadOnlyArray. This allows developers to use TypeScript const assertions for module definitions. There are no breaking changes with this commit and this change is discussed in nestjs#13326.
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. export type ModuleRef = (ModuleMetadata['imports'] & {}) extends Array<infer U> ? U : never; I feel it's worth noting that const colors: string[] = ['red'];
const doThing = (items: readonly string[]) => {};
doThing(colors); // fine So really it's only the case where you want to 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 |
Is there an existing issue that is already proposing this?
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
andforRootAsync
, 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.The text was updated successfully, but these errors were encountered: