feat($compile): added new method strictComponentBindingsEnabled #16129

ZitaNemeckova · 2017-07-25T10:55:37Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Add method strictComponentBindingsEnabled to $compile.

What is the current behavior? (You can also link to an open issue here)
Closes #16126

What is the new behavior (if this is a feature change)?
If enabled, the compiler will enforce that for all bindings of a component that are not set as optional with ?, an attribute needs to be provided on the component's HTML tag. Default value is false.

Does this PR introduce a breaking change?
Nope.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

gkalpak

This looks promising 😃 Thx!
We need some tests though.

gkalpak · 2017-07-25T13:19:05Z

docs/content/error/$compile/missingattr.ngdoc

+@fullName Missing required attribute
+@description
+
+This error may occur only when `$compileProvider.strictComponentBindingsEnabled` is set to `true`.han all attributes without `?` must be set. If one or more aren't set than first one will throw an error.


There are some typos in this sentence. Can you also limit lines to 100 chars max.

gkalpak · 2017-07-25T13:23:58Z

src/ng/compile.js

@@ -3526,6 +3570,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
            break;

          case '&':
+            if (!hasOwnProperty.call(attrs, attrName) && !optional && strictComponentBindingsEnabled) {


Nit: Check the booleans first to avoid unnecessary method calls. E.g.:

if (strictComponentBindingsEnabled && !optional && !hasOwnProperty.call(attrs, attrName)) {...

gkalpak · 2017-07-25T13:24:49Z

src/ng/compile.js

@@ -3526,6 +3570,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
            break;

          case '&':
+            if (!hasOwnProperty.call(attrs, attrName) && !optional && strictComponentBindingsEnabled) {
+              throw $compileMinErr('missingattr',


I would consider encapsulating this into a function or something, to avoid repeating it 4 times.

ZitaNemeckova · 2017-07-25T14:53:45Z

@gkalpak Thanks for review :) issues resolved and some tests added :)

gkalpak

Thx for the updates. Left a couple more comments.

The tests look good. We need some more 😃
E.g. that optional bindings do not throw even if strictComponentBindingsEnabled is true.

gkalpak · 2017-07-25T19:07:58Z

test/ng/compileSpec.js

+                      '<div prototype-method-name-as-scope-var-a></div>'
+                    )($rootScope);
+                  };
+                  expect(func).toThrowMinErr('$compile', 'missingattr', 'Attribute \'valueOf\' of \'prototypeMethodNameAsScopeVarA\' is non-optional and must be set!');


Please, limit all lines to 100 chars.

gkalpak · 2017-07-25T19:09:27Z

test/ng/compileSpec.js

+                });
+            });
+
+            it('should throw an error for undefined non-optional "&" bindings when strictComponentBindingsEnabled is true', function() {


Same description as above 😕

jeserkin · 2017-07-26T08:09:49Z

Can't wait to try this out 😄

ZitaNemeckova · 2017-07-26T12:22:05Z

@gkalpak Thanks for review :) All issues resolved and more tests added :)

gkalpak · 2017-07-27T08:01:44Z

test/ng/compileSpec.js

+              template: '<span></span>'
+            };
+          });
+          directive('optionalBindingsVarA', function() {


Couldn't you use the constructor optional binding of prototypeMethodAsScopeVar<X>?

gkalpak · 2017-07-27T08:01:52Z

test/ng/compileSpec.js

+                    'missingattr',
+                    'Attribute \'valueOf\' of \'prototypeMethodNameAs' +
+                    'ScopeVarA\' is non-optional and must be set!');
+                });


Please, also verify that it doesn't fail when the attribute is present.

gkalpak · 2017-07-27T08:08:41Z

test/ng/compileSpec.js

+                });
+            });
+
+            it('should handle "=" undefined optional bindings when strictComponentBindingsEnabled is true',


Change these descriptions to "should not throw an error for undefined optional "=" bindings when strictComponentBindingsEnabled is true".

gkalpak · 2017-07-27T08:09:48Z

test/ng/compileSpec.js

+                  });
+              });
+
+            it('should throw an error for undefined non-optional ">" bindings when ' +


Change > to < (here and below) 😃

ZitaNemeckova · 2017-07-27T11:24:57Z

@gkalpak Again thanks for review :) All issues resolved and some more tests added :D

jeserkin · 2017-07-27T11:29:27Z

@ZitaNemeckova you haven't pushed latest changes yet, right?

ZitaNemeckova · 2017-07-27T11:51:09Z

@jeserkin I did. Did I miss something from review?

jeserkin · 2017-07-27T11:56:31Z

I see only changes from 2 days ago. I might be looking in the wrong place though.

Narretz · 2017-07-27T12:00:51Z

When you amend a commit and force push it, the original commit date is kept. Is that why you think the changes are from 2 days ago?
The code has definitely changed, as is visible from the fact that not all of @gkalpak notes are not shown (because the lines have changed)

jeserkin · 2017-07-27T12:15:15Z

@Narretz correct. Have to start using github more often 😄

gkalpak

LGTM

Closes #16129

googlebot added the cla: yes label Jul 25, 2017

ZitaNemeckova force-pushed the required_attr branch from 4323a99 to e34cb82 Compare July 25, 2017 11:48

gkalpak suggested changes Jul 25, 2017

View reviewed changes

gkalpak added component: $compile frequency: moderate needs: test severity: inconvenient type: feature labels Jul 25, 2017

gkalpak added this to the Backlog milestone Jul 25, 2017

Narretz mentioned this pull request Jul 25, 2017

fix($compile): throw an exception when an attribute is unassigned #6064

Closed

ZitaNemeckova force-pushed the required_attr branch 2 times, most recently from f6d7010 to 905a4ff Compare July 25, 2017 14:51

ZitaNemeckova changed the title ~~WIP: feat($compile): added new method strictComponentBindingsEnabled~~ feat($compile): added new method strictComponentBindingsEnabled Jul 25, 2017

gkalpak suggested changes Jul 25, 2017

View reviewed changes

ZitaNemeckova force-pushed the required_attr branch from 905a4ff to 9ce340a Compare July 26, 2017 06:28

ZitaNemeckova force-pushed the required_attr branch 2 times, most recently from 6053c8d to a685440 Compare July 26, 2017 11:12

gkalpak suggested changes Jul 27, 2017

View reviewed changes

feat($compile): added new method strictComponentBindingsEnabled

6a33834

ZitaNemeckova force-pushed the required_attr branch from a685440 to 6a33834 Compare July 27, 2017 09:30

ZitaNemeckova closed this Jul 27, 2017

ZitaNemeckova reopened this Jul 27, 2017

gkalpak approved these changes Jul 28, 2017

View reviewed changes

gkalpak removed the needs: test label Jul 28, 2017

petebacondarwin approved these changes Jul 28, 2017

View reviewed changes

gkalpak closed this in f1d01bb Jul 28, 2017

gkalpak pushed a commit that referenced this pull request Jul 28, 2017

feat($compile): add strictComponentBindingsEnabled() method

3ec1819

Closes #16129

This was referenced Jul 29, 2017

Update upstream feat($compile): add strictComponentBindingsEnabled() method angular-indonesia/angular.js#55

Merged

feat($compile): add strictComponentBindingsEnabled() method GulajavaMinistudio/angular.js#51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($compile): added new method strictComponentBindingsEnabled #16129

feat($compile): added new method strictComponentBindingsEnabled #16129

ZitaNemeckova commented Jul 25, 2017 •

edited

Loading

gkalpak left a comment

gkalpak Jul 25, 2017

gkalpak Jul 25, 2017

gkalpak Jul 25, 2017

ZitaNemeckova commented Jul 25, 2017

gkalpak left a comment

gkalpak Jul 25, 2017

gkalpak Jul 25, 2017

jeserkin commented Jul 26, 2017 •

edited

Loading

ZitaNemeckova commented Jul 26, 2017

gkalpak Jul 27, 2017

gkalpak Jul 27, 2017

gkalpak Jul 27, 2017

gkalpak Jul 27, 2017

ZitaNemeckova commented Jul 27, 2017

jeserkin commented Jul 27, 2017

ZitaNemeckova commented Jul 27, 2017

jeserkin commented Jul 27, 2017

Narretz commented Jul 27, 2017

jeserkin commented Jul 27, 2017

gkalpak left a comment

feat($compile): added new method strictComponentBindingsEnabled #16129

feat($compile): added new method strictComponentBindingsEnabled #16129

Conversation

ZitaNemeckova commented Jul 25, 2017 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Jul 25, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeserkin commented Jul 26, 2017 • edited Loading

ZitaNemeckova commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Jul 27, 2017

jeserkin commented Jul 27, 2017

ZitaNemeckova commented Jul 27, 2017

jeserkin commented Jul 27, 2017

Narretz commented Jul 27, 2017

jeserkin commented Jul 27, 2017

gkalpak left a comment

Choose a reason for hiding this comment

ZitaNemeckova commented Jul 25, 2017 •

edited

Loading

jeserkin commented Jul 26, 2017 •

edited

Loading