Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dfanara
Copy link

@dfanara dfanara commented Jun 6, 2024

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?

  • 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?

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?

  • Yes
  • No

Other information

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.
@dfanara
Copy link
Author

dfanara commented Jun 6, 2024

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.

@coveralls
Copy link

coveralls commented Jun 6, 2024

Pull Request Test Coverage Report for Build f9bc21b9-dc31-40b9-a29a-6e907ff79d17

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.207%

Totals Coverage Status
Change from base Build 0a70a393-e94d-4204-995d-b0e19a5c9480: 0.0%
Covered Lines: 6744
Relevant Lines: 7314

💛 - Coveralls

Copy link
Member

@micalevisk micalevisk left a 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

@micalevisk
Copy link
Member

I just noticed something that would break due to that ReadonlyArray:

const imports: ModuleMetadata['imports'] = [];
imports.push(class {})

because we can't invoke .push on readonly arrays

and I believe that we shouldn't prevent that usage

@dfanara
Copy link
Author

dfanara commented Jun 12, 2024

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?

@micalevisk
Copy link
Member

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 imports array dynamically in a dynamic module, and you can leverage on that type definition to properly type your array

@micalevisk micalevisk self-requested a review June 26, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants