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

fix(ivy): deps are actually supported #28076

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@cexbrayat
Copy link
Contributor

cexbrayat commented Jan 11, 2019

This code was throwing if the deps array of a provider has several elements, but at the next line it resolves them... With this check ngtsc couldn’t compile ng-bootstrap for example.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The deps resolution is working but the guard stayed in place. The ngcc compilation of a module like ng-bootstrap fails with 7.2.0.

Quoting @alxhub on Slack (back in November when I first spotted the issue): Yeah, I think I wrote getDep later on when working on the same PR, and just forgot to remove the guard..

What is the new behavior?

The ngcc compilation of a module like ng-bootstrap succeeds.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@cexbrayat cexbrayat requested a review from angular/fw-compiler as a code owner Jan 11, 2019

@googlebot googlebot added the cla: yes label Jan 11, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Jan 11, 2019

Ideally it would be great if we had a test that demonstrated the bug. Any chance you could knock one up? Perhaps in ngtsc_spec.ts?

@cexbrayat

This comment has been minimized.

Copy link
Contributor

cexbrayat commented Jan 11, 2019

I added a bunch of tests because there was none for:

  • module with providers using useFactory
  • module with providers using useFactory and deps
  • @Injectable with providedIn
  • @Injectable with providedIn using useFactory
  • @Injectable with providedIn using useFactory and deps
    Only the last one is fixed by my change.

@cexbrayat cexbrayat force-pushed the cexbrayat:fix/ivy-deps branch from c099a73 to e94e788 Jan 11, 2019

@petebacondarwin
Copy link
Member

petebacondarwin left a comment

Love the tests!

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 13, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 15, 2019

@cexbrayat I've run tests in Google codebase and faced some issues. The problems are most likely cause by the fact that this PR was created on top of master while it was in broken state. Could you please rebase it based on the most up-to-date version of master and send a comment here? I'll run Google tests again to see if that resolves the problem. Thank you.

fix(ivy): deps are actually supported
This code was throwing if the `deps` array of a provider has several elements, but at the next line it resolves them... With this check `ngtsc` couldn’t compile `ng-bootstrap` for example.

@cexbrayat cexbrayat force-pushed the cexbrayat:fix/ivy-deps branch from e94e788 to b5d8637 Jan 15, 2019

@cexbrayat

This comment has been minimized.

Copy link
Contributor

cexbrayat commented Jan 15, 2019

@AndrewKushnir I've rebased the PR on latest master as you requested 👌

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 15, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 15, 2019

Thanks for rebasing @cexbrayat! I've started another g3 presubmit and will report the results here once tests are complete.

wKoza added a commit to wKoza/angular that referenced this pull request Jan 18, 2019

fix(ivy): deps are actually supported (angular#28076)
This code was throwing if the `deps` array of a provider has several elements, but at the next line it resolves them... With this check `ngtsc` couldn’t compile `ng-bootstrap` for example.

PR Close angular#28076

wKoza added a commit to wKoza/angular that referenced this pull request Jan 18, 2019

fix(ivy): deps are actually supported (angular#28076)
This code was throwing if the `deps` array of a provider has several elements, but at the next line it resolves them... With this check `ngtsc` couldn’t compile `ng-bootstrap` for example.

PR Close angular#28076
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment