formly-js / angular-formly

JavaScript powered forms for AngularJS
http://docs.angular-formly.com
MIT License
2.23k stars 405 forks source link

fix relative string models in hideExpression and watchers #639

Closed kwypchlo closed 8 years ago

kwypchlo commented 8 years ago

What

Previously discussed on my previous PR https://github.com/formly-js/angular-formly/pull/599#issuecomment-172775380

expect breaking change

String hideExpression and watchers were evaluated in context of $scope.model unlike expressionProperties that were evaluated in the context of the model of a field. That said when you had nested model like const model = {nested: {foo: 'bar'}}; and your field would have model declared as a string model: 'model.nested' your hideExpression and watcher would vary from expressionProperties:

hideExpression: 'model.nested.foo === "bar"'
expressionProperties: {
  'some.property': 'model.foo === "bar"'
},
watcher: {
  expression: 'model.nested.foo === "bar"',
  listener: function() {}
}

After my tweaks, watchers and hideExpressions that are string-based, are evaluated in the same model context as expressionProperties which is only logical.

hideExpression: 'model.foo === "bar"'
expressionProperties: {
  'some.property': 'model.foo === "bar"'
},
watcher: {
  expression: 'model.foo === "bar"',
  listener: function() {}
}

Why

I have added 2 things:

Checklist:

I might add a test or two to confirm watchers behavior but wanted to post this PR for first review

codecov-io commented 8 years ago

Current coverage is 96.03%

Merging #639 into master will increase coverage by +0.02% as of d1b47ba

@@            master    #639   diff @@
======================================
  Files           16      16       
  Stmts         1155    1160     +5
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           1109    1114     +5
  Partial          0       0       
  Missed          46      46       

Review entire Coverage Diff as of d1b47ba


Uncovered Suggestions

  1. +0.34% via ...alidationMessages.js#25...28
  2. +0.26% via src/test.utils.js#38...40
  3. +0.26% via ...s/formlyUsability.js#18...20
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

kentcdodds commented 8 years ago

Hi @kwypchlo!

Thanks for making this. I'll review it more closely soon. But if you could double check the commit message conventions, I don't think this follows it. Also, pay close attention to what you need to do in the commit message if you're introducing a breaking change please. Thanks!

kwypchlo commented 8 years ago

Yeah @kentcdodds I've just edited the message :) it took me a while to prepare this PR and I noticed I didn't conform with commit message standard just after I pushed the PR.

kentcdodds commented 8 years ago

Awesome. Thanks for doing that! I'll get back to you soon hopefully!

kentcdodds commented 8 years ago

I think this PR is great other than the one note that I mentioned. Let me know if you have questions about that.

kwypchlo commented 8 years ago

@kentcdodds yeah sorry I'm just super busy right now :) You are right, I will refactor this piece of code when I get a chance and ping you afterwards

kwypchlo commented 8 years ago

@kentcdodds I've updated this PR with some refactoring. Now field.model will always be initialized on initModel so we'll always have good model reference through field.model in form context.

kentcdodds commented 8 years ago

Looks good to me! Thoughts @formly-js/angular-formly-collaborators-read?

kentcdodds commented 8 years ago

Hopefully the auto-release works for this one. We've had a bit of trouble with that recently.

kwypchlo commented 8 years ago

cool, thanks @kentcdodds btw any idea why the changelog is not getting automatically updated with new releases? https://github.com/formly-js/angular-formly/releases is there new changelog where such breaking change as the one here would be documented?

kentcdodds commented 8 years ago

Nope :-( #643 is tracking issues with the auto-release process...