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): validate props and attrs with "on" prefix at runtime #28054

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jan 10, 2019

Prior to this change we performed prop and attr name validation at compile time, which failed in case a given prop/attr is an input to a Directive (thus should not be a subject to this check). Since Directive matching in Ivy happens at runtime, the corresponding checks are now moved to runtime as well.

Messages in validateProperty and validateAttribute functions functions were taken from the corresponding functions in Compiler, to make sure all asserts remain the same.

This PR resolves FW-786.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir requested a review from kara Jan 10, 2019

@AndrewKushnir AndrewKushnir requested review from angular/fw-compiler as code owners Jan 10, 2019

@ngbot ngbot bot added this to the needsTriage milestone Jan 10, 2019

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

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

if (parts.length > 1) {
if (parts[0] == ATTRIBUTE_PREFIX) {
boundPropertyName = parts[1];
this._validatePropertyOrAttributeName(boundPropertyName, boundProp.sourceSpan, true);
if (!skipValidation) {

This comment has been minimized.

@pkozlowski-opensource

pkozlowski-opensource Jan 11, 2019

Member

nit: consider removing double negation (rename to sth like validateAttrOrProp and remove !). While we are at the names maybe we should be explicit what the validation is all about. validateName?

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource left a comment

LGTM.

Left one nit, feel free to ignore.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 11, 2019

@kara

kara approved these changes Jan 11, 2019

Copy link
Contributor

kara left a comment

LGTM

AndrewKushnir added some commits Jan 10, 2019

fix(ivy): validate props and attrs with "on" prefix at runtime
Prior to this change we performed prop and attr name validation at compile time, which failed in case a given prop/attr is an input to a Directive (thus should not be a subject to this check). Since Directive matching in Ivy happens at runtime, the corresponding checks are now moved to runtime as well.

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-786_on_prefixed_props_v2 branch from b6f6117 to 8535a52 Jan 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 12, 2019

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jan 14, 2019

fix(ivy): validate props and attrs with "on" prefix at runtime (angul…
…ar#28054)

Prior to this change we performed prop and attr name validation at compile time, which failed in case a given prop/attr is an input to a Directive (thus should not be a subject to this check). Since Directive matching in Ivy happens at runtime, the corresponding checks are now moved to runtime as well.

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