-
-
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
refactor(common,core): support read-only arrays on module metadata #13655
Conversation
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.
Are there additional test cases you would like to see? I added a test case for NestContainer since that was a direct change to the function definition, however, I wasn't sure if a test was necessary for the ModuleMetadata interface changes. |
Pull Request Test Coverage Report for Build f9bc21b9-dc31-40b9-a29a-6e907ff79d17Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that Array<T>
wouldn't allow those arrays but I was wrong
I just noticed something that would break due to that const imports: ModuleMetadata['imports'] = [];
imports.push(class {}) because we can't invoke and I believe that we shouldn't prevent that usage |
Ah yeah, I wasn't sure about that. Should something like the nest container be able to modify an input array in that use case? I'm not too familiar with the internals (thought this might be a good first issue to really start learning more about that), but in general it seems like pushing an item onto that array might cause some unintended side effects? |
I don't think so But I was talking about a valid client-land use case. Imagine that you want to define the |
as @micalevisk pointed out above, we sometimes need to dynamically update the module's metadata (create an object first and then fill it with refs) |
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 #13326.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #13326
What is the new behavior?
ReadOnlyArray is not supported to better support using TypeScript const assertions in module definitions.
Does this PR introduce a breaking change?
Other information