The Wayback Machine - https://web.archive.org/web/20210805171939/http://github.com/angular/angular/pull/42601
Skip to content

/ angular

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

Jump to bottom

fix(compiler-cli): check split two way binding #42601

Closed
wants to merge 1 commit into from
Closed

fix(compiler-cli): check split two way binding #42601

wants to merge 1 commit into from
+217 −28

Conversation

@danieltre23
Copy link
Contributor

@danieltre23 danieltre23 commented Jun 18, 2021

Check for split two way binding when output is not declared to make error message clearer.

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?

Issue Number: 39300

What is the new behavior?

The new error message for a banana-in-a-box binding with the output not declared should be "The output {outputName} for the two way binding is not declared."

Does this PR introduce a breaking change?

  • Yes
  • No

Other information
@danieltre23 danieltre23 requested review from atscott, alxhub and dgp1130 Jun 18, 2021
@google-cla google-cla bot added the cla: yes label Jun 18, 2021
@ngbot ngbot bot added this to the Backlog milestone Jun 18, 2021
@dgp1130
dgp1130 reviewed Jun 18, 2021
View changes
Copy link
Contributor

@dgp1130 dgp1130 left a comment

Great job Daniel! This looks really cool. Just a few nitpicks and a couple edge cases to consider and this should be good to go.

One general nit: Prefer const over let wherever possible, just to reduce the possibility of mutation where its not needed.
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch 3 times, most recently from 56c4e75 to 52f398b Jun 21, 2021
@danieltre23 danieltre23 changed the title fix(complier-cli): check split two way binding fix(compiler-cli): check split two way binding Jun 21, 2021
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch 5 times, most recently from 3f904ca to 64e39e9 Jun 21, 2021
@dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Jun 22, 2021

All the code looks good to me, and I kicked off a google3 presubmit, but it looks like it has a few legitimate failures: http://test/OCL:380647145:BASE:380647172:1624306026646:e07d2f4f.

Looks like there's a lot of code which two-way binds to model properties that don't have a modelChange event. I'm surprised this wasn't an error before, do we special case the model property in some way? I'm wondering if we should exempt it from this check or else we'll likely need to roll this out behind a flag. Thoughts, @alxhub?
@danieltre23
Copy link
Contributor Author

@danieltre23 danieltre23 commented Jun 22, 2021

All the code looks good to me, and I kicked off a google3 presubmit, but it looks like it has a few legitimate failures: http://test/OCL:380647145:BASE:380647172:1624306026646:e07d2f4f.

Looks like there's a lot of code which two-way binds to model properties that don't have a modelChange event. I'm surprised this wasn't an error before, do we special case the model property in some way? I'm wondering if we should exempt it from this check or else we'll likely need to roll this out behind a flag. Thoughts, @alxhub?

I think the error should only show when the strictTemplates flag is true, I noticed that the error in the issue #39300 is only for strictTemplates, so I'll add that check.
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch from 64e39e9 to dc5cc60 Jun 22, 2021
@dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Jun 22, 2021

Passing presubmit: http://test/OCL:380647145:BASE:380826857:1624379963990:c3d8b02d
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch from dc5cc60 to c4187e7 Jun 22, 2021
@dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Jun 23, 2021

Ran a global presubmit since strict templates is not widely used internally and I wanted to make sure we were checking at least some strict templates targets, everything looks good: http://test/OCL:380970929:BASE:380971055:1624432874430:81de2e8d.
@dgp1130
dgp1130 approved these changes Jun 24, 2021
View changes
Copy link
Contributor

@dgp1130 dgp1130 left a comment

Forgot to approve after the google3 tests passed.
@alxhub
alxhub requested changes Jun 24, 2021
View changes
Copy link
Contributor

@alxhub alxhub left a comment

Overall I think the mechanics of this PR are solid. Let's tweak the error message a bit to more accurately reflect the situation in the template, and then this should be good to go.
packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts Outdated Show resolved Hide resolved
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch from c4187e7 to 77a5a35 Jun 28, 2021
@danieltre23 danieltre23 requested a review from alxhub Jun 28, 2021
@alxhub
alxhub reviewed Jun 28, 2021
View changes
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
@alxhub
alxhub reviewed Jun 28, 2021
View changes
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
@alxhub
alxhub reviewed Jun 28, 2021
View changes
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
@danieltre23 danieltre23 requested a review from alxhub Jul 2, 2021
@danieltre23
Copy link
Contributor Author

@danieltre23 danieltre23 commented Jul 2, 2021

Flaky failures in presubmit: http://test/OCL:382577037:BASE:382802023:1625257425653:73bd3481 forced green.
@alxhub
alxhub requested changes Jul 8, 2021
View changes
Copy link
Contributor

@alxhub alxhub left a comment

Last round of comments - after this it should be good to go!

Also, even though this is an unlikely error, it's still a new error and thus we should only land it in a minor version instead of a patch.
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch from 8b1379c to 2b837b1 Jul 9, 2021
@danieltre23
Copy link
Contributor Author

@danieltre23 danieltre23 commented Jul 9, 2021

Flaky failures in presubmit: http://test/OCL:382577037:BASE:383866895:1625849540387:7301e2db forced green.
@alxhub
alxhub approved these changes Jul 9, 2021
View changes
packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested review from atscott, jelbourn, jessicajaniuk and zarend Jul 9, 2021
@alxhub
Copy link
Contributor

@alxhub alxhub commented Jul 9, 2021

Reviewed-for: public-api
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch from 2b837b1 to 26922ec Jul 9, 2021
@atscott
atscott approved these changes Jul 9, 2021
View changes
Copy link
Contributor

@atscott atscott left a comment

reviewed-for: public-api
@pullapprove pullapprove bot requested a review from petebacondarwin Jul 9, 2021
@jessicajaniuk
jessicajaniuk approved these changes Jul 9, 2021
View changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

LGTM 🍪

I left a couple of suggestions, but nothing I feel strongly about.

reviewed-for: public-api
packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from IgorMinar Jul 9, 2021
@danieltre23 Daneil Trevino
fix(compiler-cli): check split two way binding
ebad174 
Check for split two way binding when output is not declared to make error message clearer.
@danieltre23 danieltre23 force-pushed the danieltre23:split-two-way-binding branch from 26922ec to ebad174 Jul 9, 2021
@danieltre23
Copy link
Contributor Author

@danieltre23 danieltre23 commented Jul 9, 2021

@jessicajaniuk updated relatedMessages to use for-of per a separate suggestion from Alex.
@petebacondarwin
petebacondarwin approved these changes Jul 12, 2021
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Reviewed-for: public-api
@dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Jul 13, 2021

One final TGP looks good: http://test/OCL:384348576:BASE:384348514:1626141913210:709ae796
@dgp1130 dgp1130 removed request for IgorMinar, jelbourn and zarend Jul 13, 2021
@zarend
Copy link
Contributor

@zarend zarend commented Jul 13, 2021

👍 🍌 📦💥
@danieltre23 danieltre23 mentioned this pull request Jul 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petebacondarwin petebacondarwin

@atscott atscott

@alxhub alxhub

@dgp1130 dgp1130

@jessicajaniuk jessicajaniuk

Assignees
No one assigned
Labels
action: merge cla: yes comp: compiler target: minor
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@danieltre23 @dgp1130 @alxhub @zarend @petebacondarwin @atscott @jessicajaniuk @dylhunn