fix(core): allow readonly arrays for standalone imports#47851
fix(core): allow readonly arrays for standalone imports#47851pkozlowski-opensource wants to merge 1 commit intoangular:mainfrom
Conversation
There was a problem hiding this comment.
What I don't like with this change is that it breaks symmetry with @NgModule.imports. Should we change the type in there as well?
Also, not sure if I should use readonly vs Readonly<...> - what do we prefer in terms of style?
readonly(Type<any>|readonly any[])[]; vs. Readonly<(Type<any>|Readonly<any[]>)[]>;
There was a problem hiding this comment.
Ended up going with ReadonlyArray<Type<any> | ReadonlyArray<any>>
7463c0d to
4b92d06
Compare
There was a problem hiding this comment.
Drive-by comment, feel free to ignore: if we're doing this for imports, should we do the same for the other array properties on the decorators?
There was a problem hiding this comment.
Yep, this is an open question for me as well.
There was a problem hiding this comment.
In all those cases it would make sense to allow read-only arrays. The end game is combining everything in there, not mutating any of the given parameters.
There was a problem hiding this comment.
So, finally I've changed the type to imports?: (Type<any>|ReadonlyArray<any>)[]; to only allow read-only arrays in the nested position only. The only other place, in the @Component decorator where we've got this situation is entryComponents and this one is deprecated.
The consistency with the NgModule is still a concern.
jessicajaniuk
left a comment
There was a problem hiding this comment.
reviewed-for: fw-core, public-api
dylhunn
left a comment
There was a problem hiding this comment.
reviewed-for: fw-core, public-api
Standalone components should support readonly arrays in the `@Component.imports`. Fixes angular#47643
4b92d06 to
0a0be28
Compare
|
@AndrewKushnir thnx for the review - I've ended up adjusting the type so the problem / suggestion are no longer relevant. |
@pkozlowski-opensource quick question: should we consider this use-case as well (not a blocker for this PR, just wondering)? |
@AndrewKushnir this is an open question for me - no strong feelings here, just went with a simpler solution as none of the existing APIs allows top-level readonly arrays. So if we want to do it I would rather adjust them all in a separate PR. But also happy to change it just here. |
@pkozlowski-opensource that makes sense, thanks 👍 I'd vote to proceed with the current implementation and change top-level array types later if needed. |
|
This PR was merged into the repository by commit 2d085dc. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Standalone components should support readonly arrays in the `@Component.imports`. Fixes angular#47643 PR Close angular#47851
Standalone components should support readonly arrays in the
@Component.imports.Fixes #47643