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.

Sign up for GitHub

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

Jump to bottom

fix(core): allow readonly arrays for standalone imports #47851

Closed
wants to merge 1 commit into from
Closed

fix(core): allow readonly arrays for standalone imports #47851

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

Standalone components should support readonly arrays in the @Component.imports.

Fixes #47643

@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime cross-cutting: standalone Issues related to the NgModule-less world labels Oct 24, 2022
@ngbot ngbot bot modified the milestone: Backlog Oct 24, 2022
pkozlowski-opensource
pkozlowski-opensource commented Oct 24, 2022
View reviewed changes
packages/core/src/metadata/directives.ts Outdated
@@ -629,7 +629,7 @@ export interface Component extends Directive {
* More information about standalone components, directives, and pipes can be found in [this
* guide](guide/standalone-components).
*/
imports?: (Type<any>|any[])[];
imports?: readonly(Type<any>|readonly any[])[];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[]>)[]>;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up going with ReadonlyArray<Type<any> | ReadonlyArray<any>>

crisbeto
crisbeto reviewed Oct 24, 2022
View reviewed changes
packages/core/src/metadata/directives.ts Outdated
@@ -629,7 +629,7 @@ export interface Component extends Directive {
* More information about standalone components, directives, and pipes can be found in [this
* guide](guide/standalone-components).
*/
imports?: (Type<any>|any[])[];
imports?: ReadonlyArray<Type<any>|ReadonlyArray<any>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is an open question for me as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review October 24, 2022 18:37
jessicajaniuk
jessicajaniuk approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: fw-core, public-api

dylhunn
dylhunn approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: fw-core, public-api

@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 25, 2022
@pkozlowski-opensource
fix(core): allow readonly arrays for standalone imports
0a0be28 
Standalone components should support readonly arrays in the
`@Component.imports`.

Fixes angular#47643
@pkozlowski-opensource
Copy link
Member Author

@AndrewKushnir thnx for the review - I've ended up adjusting the type so the problem / suggestion are no longer relevant.

@AndrewKushnir
Copy link
Contributor

I've ended up #47851 (comment) so the problem / #47851 (comment) are no longer relevant.

@pkozlowski-opensource quick question: should we consider this use-case as well (not a blocker for this PR, just wondering)?

    const DirAndPipe = [RedIdDirective, BluePipe] as const;

    @Component({
      imports: DirAndPipe, // no wrapping around const array
      ...
    })
    class TestComponent {}

@pkozlowski-opensource
Copy link
Member Author

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.

@AndrewKushnir
Copy link
Contributor

no strong feelings here, just went with a simpler solution as none of the existing APIs allows top-level readonly arrays

@pkozlowski-opensource that makes sense, thanks 👍 I'd vote to proceed with the current implementation and change top-level array types later if needed.

@pkozlowski-opensource pkozlowski-opensource removed the request for review from alxhub October 26, 2022 16:36
@pkozlowski-opensource pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 26, 2022
@pkozlowski-opensource
Copy link
Member Author

This PR was merged into the repository by commit 2d085dc.

pkozlowski-opensource added a commit that referenced this pull request Oct 27, 2022
@pkozlowski-opensource
fix(core): allow readonly arrays for standalone imports (#47851)
96c0e42 
Standalone components should support readonly arrays in the
`@Component.imports`.

Fixes #47643

PR Close #47851
nouraellm pushed a commit to nouraellm/angular that referenced this pull request Nov 6, 2022
@pkozlowski-opensource @nouraellm
fix(core): allow readonly arrays for standalone imports (angular#47851)
7aac725 
Standalone components should support readonly arrays in the
`@Component.imports`.

Fixes angular#47643

PR Close angular#47851
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 27, 2022
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@pkozlowski-opensource @trekladyone
fix(core): allow readonly arrays for standalone imports (angular#47851)
b26014d 
Standalone components should support readonly arrays in the
`@Component.imports`.

Fixes angular#47643

PR Close angular#47851
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SanderElias SanderElias SanderElias left review comments

@crisbeto crisbeto crisbeto left review comments

@dylhunn dylhunn dylhunn approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cross-cutting: standalone Issues related to the NgModule-less world target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
v15 final blockers
Development

Successfully merging this pull request may close these issues.

Array of cooperating directives throws error with standalone flag on
6 participants
@pkozlowski-opensource @AndrewKushnir @SanderElias @crisbeto @dylhunn @jessicajaniuk