Skip to content

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

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
Closed
pkozlowski-opensource wants to merge 1 commit into from
Closed

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

pkozlowski-opensource wants to merge 1 commit into from

Conversation

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Oct 24, 2022

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
Copy link
Member Author

@pkozlowski-opensource pkozlowski-opensource Oct 24, 2022 •
edited
Loading

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

@pkozlowski-opensource pkozlowski-opensource Oct 24, 2022

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

pkozlowski-opensource
pkozlowski-opensource commented Oct 24, 2022
View reviewed changes
crisbeto
crisbeto reviewed Oct 24, 2022
View reviewed changes
packages/core/src/metadata/directives.ts Outdated
Copy link
Member

@crisbeto crisbeto Oct 24, 2022

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

@pkozlowski-opensource pkozlowski-opensource Oct 24, 2022

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

@SanderElias SanderElias Oct 25, 2022

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

@pkozlowski-opensource pkozlowski-opensource Oct 25, 2022

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

@pullapprove pullapprove bot requested a review from AndrewKushnir October 24, 2022 20:58
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

AndrewKushnir
AndrewKushnir approved these changes Oct 25, 2022
View reviewed changes
@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

pkozlowski-opensource commented Oct 25, 2022

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

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Oct 25, 2022

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

pkozlowski-opensource commented Oct 26, 2022

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

AndrewKushnir commented Oct 26, 2022

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

pkozlowski-opensource commented Oct 27, 2022

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
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Nov 27, 2022

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

@crisbeto crisbeto crisbeto left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+3 more reviewers

@SanderElias SanderElias SanderElias left review comments

@dylhunn dylhunn dylhunn approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Reviewers whose approvals may not affect merge requirements

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