Skip to content
This repository has been archived by the owner on Apr 30, 2018. It is now read-only.

fix relative string models in hideExpression and watchers #639

Merged

Conversation

kwypchlo
Copy link
Member

What

Previously discussed on my previous PR #599 (comment)

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

  • hideExpression and expressionProperties should be as similar to developer as possible - the mechanism behind them can be different but their api and behavior should be the same thus expression copied from expressionProperty should work with hideExpression and vice versa
  • watchExpression was by default 'model.' + key which didn't work out of a box for nested models because fe. const model = {nested: {foo: 'bar'}}; with model: 'model.nested', watcherExpression would be model.foo because of watchExpression = 'model[\'' + field.key.toString().split('.').join('\'][\'') + '\']' and this would not work because it didn't take into account that model was moved to nested child and would expect model.nested.foo to work

How

I have added 2 things:

  • in method getFormlyFieldLikeLocals that prepares context for field expressions registered in formly-form controller to look more like these registered from the formly-field controller. getFormlyFieldLikeLocals has one time that field.model is not yet ready - this is on initModel method call that actually resolves field.model and should be run in $scope.model context. Other than that, anything that uses getFormlyFieldLikeLocals should be run with model as field.model.
  • watchExpression in formly watcher registration gets run through $parse to add field.model as a model

Checklist:

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

@codecov-io
Copy link

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
Copy link
Member

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 kwypchlo force-pushed the fix-relative-model-in-expressions branch from f9e389d to e68c568 Compare February 18, 2016 23:14
@kwypchlo
Copy link
Member Author

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
Copy link
Member

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

@@ -366,6 +368,9 @@ function formlyForm(formlyUsability, formlyWarn, $parse, formlyConfig, $interpol
function getFormlyFieldLikeLocals(field, index) {
// this makes it closer to what a regular formlyExpression would be
return {
// when field.model is not yet an object, this call should be evaluated in $scope.model context
// because this is a call where field.model gets resolved
model: angular.isObject(field.model) ? field.model : $scope.model,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should actually put this logic in the place where the field model gets resolved rather than here because it could lead to unexpected results in other expressions.

@kentcdodds
Copy link
Member

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

@kwypchlo
Copy link
Member Author

@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

… watcher expression for nested models

BREAKING CHANGE: reference to 'model' in string hideExpressions and watchers on fields with nested models now points to field.model just as in expressionProperties
@kwypchlo kwypchlo force-pushed the fix-relative-model-in-expressions branch from e68c568 to c899ee6 Compare February 25, 2016 21:01
@kwypchlo
Copy link
Member Author

@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
Copy link
Member

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

@kentcdodds kentcdodds mentioned this pull request Feb 25, 2016
kentcdodds pushed a commit that referenced this pull request Feb 25, 2016
fix relative string models in hideExpression and watchers
@kentcdodds kentcdodds merged commit 6cd15e1 into formly-js:master Feb 25, 2016
@kentcdodds
Copy link
Member

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

@kwypchlo
Copy link
Member Author

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?

@kwypchlo kwypchlo deleted the fix-relative-model-in-expressions branch February 26, 2016 04:38
@kentcdodds
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants