Skip to content

Conversation

pkozlowski-opensource
Copy link
Member

NgModule should support readonly arrays in @NgModule.imports and @NgModule.exports.

Partly fixes #48089 (#48091 is also needed)

@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release cross-cutting: standalone Issues related to the NgModule-less world labels Nov 17, 2022
@ngbot ngbot bot modified the milestone: Backlog Nov 17, 2022
@pkozlowski-opensource
Copy link
Member Author

Note to a reviewer: I'm not sure if we can make this change in the patch release as technically it is widening the API surface. On the other hand it could be seen as a fix. On a fence here.

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review November 17, 2022 14:35
@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 17, 2022
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.

LGTM

reviewed-for: fw-core, public-api

@pullapprove pullapprove bot requested a review from AndrewKushnir November 17, 2022 19:22
Copy link
Contributor

@AndrewKushnir AndrewKushnir 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: public-api

@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 7, 2022
@ngbot
Copy link

ngbot bot commented Dec 7, 2022

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending status "mergeability" is pending
    pending 2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@atscott atscott added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release action: merge The PR is ready for merge by the caretaker labels Dec 7, 2022
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 7, 2022
@atscott atscott added action: presubmit The PR is in need of a google3 presubmit and removed action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 7, 2022
@ngbot
Copy link

ngbot bot commented Dec 7, 2022

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending forbidden labels detected: action: cleanup
    pending 4 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@pkozlowski-opensource pkozlowski-opensource force-pushed the standalone_ng_module_standalone_imports branch from c985860 to 22e317c Compare January 2, 2023 12:29
@@ -136,7 +136,7 @@ export interface NgModule {
* ```
*
*/
imports?: Array<Type<any>|ModuleWithProviders<{}>|any[]>;
imports?: Array<Type<any>|ModuleWithProviders<{}>|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.

Should this not be:

Suggested change
imports?: Array<Type<any>|ModuleWithProviders<{}>|ReadonlyArray<any>>;
imports?: Array<Type<any>|ModuleWithProviders<{}>|ReadonlyArray<Type<any>>>;

in which case I think we can just do:

Suggested change
imports?: Array<Type<any>|ModuleWithProviders<{}>|ReadonlyArray<any>>;
imports?: ReadonlyArray<Type<any>|ModuleWithProviders<{}>;

as all Arrays are implicitly ReadonlyArrays.

Copy link

@csisy csisy Aug 3, 2023

Choose a reason for hiding this comment

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

Can Type<any> represent an array of types? Previously, it was valid to import an array directly, like this:

const myArr = [/*...*/];
import: [myArr]

IMO keeping the type as-is can be more descriptive about what the import accepts.

@hakimio
Copy link

hakimio commented Jun 1, 2023

@pkozlowski-opensource are you planning to finish this PR anytime in near future?

@pullapprove pullapprove bot removed the request for review from atscott June 13, 2023 00:30
@tomastrajan
Copy link
Contributor

Any update on this?

@hakimio
Copy link

hakimio commented Oct 3, 2023

Based on the following rejections and @pkozlowski-opensource lack of response, it seems the PR will be just forgotten until it's closed due to its age.

image

@distante
Copy link

Following this since it is still a problem on production code.

@pkozlowski-opensource pkozlowski-opensource force-pushed the standalone_ng_module_standalone_imports branch 3 times, most recently from 336e217 to 261ae4c Compare July 12, 2024 10:31
NgModule should support readonly arrays in `@NgModule.imports` and
`@NgModule.exports`.

Fixes angular#48089
@pkozlowski-opensource pkozlowski-opensource force-pushed the standalone_ng_module_standalone_imports branch from 261ae4c to bf44b7d Compare July 12, 2024 15:14
@SanderElias
Copy link
Contributor

@pkozlowski-opensource @alxhub
Having this would be a huge boon for 3rth party Angular libraries.
Is there something we can to to help with this?

@tomastrajan
Copy link
Contributor

@pkozlowski-opensource could you please provide a very short info if this was solved elsewhere or wont be solved or ? thank you!

@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 May 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: presubmit The PR is in need of a google3 presubmit area: core Issues related to the framework runtime cross-cutting: standalone Issues related to the NgModule-less world target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exported array of standalone components cannot be imported
10 participants