Skip to content

fix(datepicker): compilation errors with ViewEngine #19516

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

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 2, 2020

Fixes a few compilation errors that only happen when the date range picker is compiled with ViewEngine.

@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release labels Jun 2, 2020
@crisbeto crisbeto requested a review from mmalerba as a code owner June 2, 2020 17:58
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 2, 2020
Fixes a few compilation errors that only happen when the date range picker is compiled with `ViewEngine`.
@crisbeto crisbeto force-pushed the date-range-picker-view-engine branch from 5da747a to 86face2 Compare June 2, 2020 18:05
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba added lgtm action: merge The PR is ready for merge by the caretaker labels Jun 2, 2020
@mmalerba
Copy link
Contributor

mmalerba commented Jun 2, 2020

do we have a project or something for stuff that needs to be fixed before 10.0.0? (edit: I added it to the 10.0.0 milestone, I assume that's what its for)

@mmalerba mmalerba added this to the 10.0.0 milestone Jun 2, 2020
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

@crisbeto do you know why this hasn't been captured by our View Engine test job?

@crisbeto
Copy link
Member Author

crisbeto commented Jun 2, 2020

No idea, I randomly had to run the dev app with --config=view-engine to test something different and these errors showed up.

@devversion
Copy link
Member

@crisbeto I looked into it. I think the protected member change is reasonable in View Engine, but I'm surprised by the other one:

src/components-examples/material/datepicker/date-range-picker-overview/date-range-picker-overview-example.html(4,5): Directive MatStartDate, Property 'rangePicker' does not exist on type 'MatDateRangeInputParent<any>'.

Does that mean that Ivy doesn't properly check host bindings? Should we submit a bug report?

@crisbeto
Copy link
Member Author

crisbeto commented Jun 3, 2020

By the looks of it, all 3 failures are from host bindings.

@devversion
Copy link
Member

devversion commented Jun 3, 2020

@crisbeto yeah, but the one with the protected members doesn't seem to surface because the factories in Ivy are static properties of the directive/component. So it's possible to now use private or protected for expressions that are referenced in templates/host bindings.

Though, that wouldn't tell us whether host bindings are checked or not in Ivy. Should we look into this on the framework side? I'd expect View Engine not to be stricter than Ivy with strict templates enabled.

@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 Jul 6, 2020
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 PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants