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.

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

bug: componentProviders failed on nested array providers #506

Closed
lacolaco opened this issue Dec 12, 2024 · 4 comments · Fixed by #507
Closed

bug: componentProviders failed on nested array providers #506

lacolaco opened this issue Dec 12, 2024 · 4 comments · Fixed by #507

Comments

@lacolaco
Copy link
Contributor

lacolaco commented Dec 12, 2024

The providers in the Angular framework always flatten all elements in the case of nested arrays of elements. In other words, Provider can be an array of providers.

https://angular.dev/api/core/Provider

type Provider = | TypeProvider
  | ValueProvider
  | ClassProvider
  | ConstructorProvider
  | ExistingProvider
  | FactoryProvider
  | any[] // <--- an array of providers.

However, due to recent changes, ATL no longer accepts nested arrays in componentProviders property. Previously, array.concat(provider) had that role, but in the current implementation the step to flatten componentProviders is lost. This is a regresssion.

061d5cc#diff-8e35d38d4ce658063a1f7522f82a938a8b99a52223927129ad8ddd28df541506L124

-  componentProviders
-    .reduce((acc, provider) => acc.concat(provider), [] as any[])
-    .forEach((p: any) => {
-      const { provide, ...provider } = p;
-      TestBed.overrideProvider(provide, provider);
-    });
+  for (const { provide, ...provider } of componentProviders) {
+    TestBed.overrideProvider(provide, provider);
+  }
@lacolaco
Copy link
Contributor Author

Strictly speaking, even array.concat(provider) would not be consistent with the Angular framework's behavior, as it would not be able to flatten nesting 2+ levels, but at least as a regression fix, componentProviders.flat() seems to be enough.

    for (const provider of componentProviders.flat()) {
        TestBed.overrideProvider(provide, provider);
    }

@lacolaco
Copy link
Contributor Author

@timdeschryver How do you think the solution above; componentProviders.flat()? It has a lower risk than componentProviders.flat(Infinity) which can flatten nested arrays completely, but the incompatibility to Angular is not solved. IMO, componentProviders.flat(Infinity) is better.

@timdeschryver
Copy link
Member

@lacolaco this is a case that we haven't thought of.
Feel free to create a PR using componentProviders.flat(Infinity), and maybe update providers as well?

Sided question: since standalone is the default, do you think it would be better to rename componentProviders to providers? And the existing providers to global/applicationProviders?

@lacolaco
Copy link
Contributor Author

lacolaco commented Dec 13, 2024

@lacolaco this is a case that we haven't thought of. Feel free to create a PR using componentProviders.flat(Infinity), and maybe update providers as well?

Sided question: since standalone is the default, do you think it would be better to rename componentProviders to providers? And the existing providers to global/applicationProviders?

@timdeschryver Yeah, I've created a PR. Please review it when you have time.
About the naming, I prefer the simple providers for @Component.providers. However, frequent API changes are a burden for ATL users, and since Angular itself has included improvements to TestBed in its roadmap, I think it is not too late to wait and see how that moves forward. I feel that it is not that big of a problem even in its current state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants