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 in NgModule imports and exports #48106

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

fix(core): allow readonly arrays in NgModule imports and exports #48106

wants to merge 1 commit into from

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
jessicajaniuk
jessicajaniuk approved these changes Nov 17, 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.

LGTM

reviewed-for: fw-core, public-api

AndrewKushnir
AndrewKushnir approved these changes Nov 17, 2022
View reviewed changes
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
fix(core): allow readonly arrays in NgModule imports and exports
22e317c 
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 c985860 to 22e317c Compare January 2, 2023 12:29
@hakimio hakimio mentioned this pull request Jan 4, 2023
Open
alxhub
alxhub requested changes Jan 6, 2023
View reviewed changes
packages/core/src/metadata/ng_module.ts
@@ -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 •
edited

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.

@pkozlowski-opensource pkozlowski-opensource mentioned this pull request Apr 18, 2023
Closed
@hakimio hakimio mentioned this pull request May 28, 2023
Open
@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
@hakimio hakimio mentioned this pull request Jun 13, 2023
Closed
1 task
@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

@hakimio hakimio mentioned this pull request Nov 8, 2023
Closed
@JeanMeche JeanMeche mentioned this pull request Apr 10, 2024
Closed
@distante
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csisy csisy csisy left review comments

@alxhub alxhub alxhub requested changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

@dylhunn dylhunn Awaiting requested review from dylhunn

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
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
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

Exported array of standalone components cannot be imported
9 participants
@pkozlowski-opensource @hakimio @tomastrajan @distante @alxhub @csisy @AndrewKushnir @jessicajaniuk @atscott