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

fix(ivy): add support for optional nullable injection tokens #27552

Closed

Conversation

IgorMinar
Copy link
Contributor

FW-778 #resolve

@IgorMinar IgorMinar force-pushed the ivy/fix-fw-778-nullable-di-token branch from 426408e to 58bb089 Compare December 7, 2018 20:14
@mary-poppins
Copy link

You can preview 426408e at https://pr27552-426408e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 58bb089 at https://pr27552-58bb089.ngbuilds.io/.

@@ -185,6 +213,10 @@ function expectParameter(
}

function argExpressionToString(name: ts.Node): string {
if (name == null) {
Copy link
Member

Choose a reason for hiding this comment

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

This check seems to go against the type of this function - let's change the type to be nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

When does it return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a helper function that is called after expect(foo).not.toBeNull() which in jasmine doesn't abort the rest of the expectations, so if we don't check for nulls here, the tests blows up if that expectation fails.

@IgorMinar IgorMinar force-pushed the ivy/fix-fw-778-nullable-di-token branch from 58bb089 to c1e4163 Compare December 8, 2018 00:32
@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Dec 8, 2018
@mary-poppins
Copy link

You can preview c1e4163 at https://pr27552-c1e4163.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 0035d4b at https://pr27552-0035d4b.ngbuilds.io/.

@IgorMinar
Copy link
Contributor Author

@IgorMinar IgorMinar force-pushed the ivy/fix-fw-778-nullable-di-token branch from 0035d4b to 6169720 Compare December 12, 2018 15:34
@mary-poppins
Copy link

You can preview 6169720 at https://pr27552-6169720.ngbuilds.io/.

@kara kara added the comp: ivy label Dec 12, 2018
@ngbot ngbot bot added this to the needsTriage milestone Dec 12, 2018
@mhevery mhevery closed this in 7fabe44 Dec 12, 2018
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants