formly-js / angular-formly

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

hideExpression returning promise doesn't work #539

Closed ecgtheow closed 9 years ago

ecgtheow commented 9 years ago

jsBin link: http://jsbin.com/kabinepuya/

I'm using Angular v1.4.3. I suspect the reason it's not working is that ng-if doesn't unwrap promises any more (if it ever did.) So when the template says 'ng-if="!field.hide"', and field.hide is a promise, then it all goes wrong.

My prototype fix is attached. Basically I just wrap the call to formlyUtil.formlyEval () inside evalCloseToFormlyExpression () with $q.when (), and then set field.hide when the promise resolves. I didn't do this to the other call to evalCloseToFormlyExpression () assigning to field.model, as I don't know whether that would break anything.

If you're happy with this then I'll do a proper PR.

$ diff -u formly.js.bk formly.js
--- formly.js.bk    2015-11-04 19:05:10.789652616 +0000
+++ formly.js   2015-11-05 12:43:53.982073453 +0000
@@ -2025,7 +2025,7 @@
    // @ngInject
    function formlyForm(formlyUsability, formlyWarn, $parse, formlyConfig, $interpolate) {
      var currentFormId = 1;
-     FormlyFormController.$inject = ["$scope", "formlyApiCheck", "formlyUtil"];
+     FormlyFormController.$inject = ["$scope", "$q", "formlyApiCheck", "formlyUtil"];
      return {
        restrict: 'E',
        template: formlyFormGetTemplate,
@@ -2104,7 +2104,7 @@
      }

      // @ngInject
-     function FormlyFormController($scope, formlyApiCheck, formlyUtil) {
+     function FormlyFormController($scope, $q, formlyApiCheck, formlyUtil) {
        setupOptions();
        $scope.model = $scope.model || {};
        setupFields();
@@ -2122,7 +2122,11 @@
            if (field.hideExpression) {
              // can't use hide with expressionProperties reliably
              var val = model[field.key];
-             field.hide = evalCloseToFormlyExpression(field.hideExpression, val, field, index);
+             // Avoid flicker when hideExpression promises resolve to true
+             field.hide = true;
+             evalCloseToFormlyExpression(field.hideExpression, val, field, index, true).then (function (hide) {
+               field.hide = hide;
+             });
            }
            if (field.extras && field.extras.validateOnModelChange && field.formControl) {
              field.formControl.$validate();
@@ -2210,7 +2214,7 @@
              isNewModel = false;
            }

-           field.model = evalCloseToFormlyExpression(expression, undefined, field, index);
+           field.model = evalCloseToFormlyExpression(expression, undefined, field, index, false);
            if (!field.model) {
              throw formlyUsability.getFieldError('field-model-must-be-initialized', 'Field model must be initialized. When specifying a model as a string for a field, the result of the' + ' expression must have been initialized ahead of time.', field);
            }
@@ -2286,9 +2290,13 @@
          return [$scope.fields[index]].concat(originalArgs, [watcher.stopWatching]);
        }

-       function evalCloseToFormlyExpression(expression, val, field, index) {
+       function evalCloseToFormlyExpression(expression, val, field, index, asPromise) {
          var extraLocals = getFormlyFieldLikeLocals(field, index);
-         return formlyUtil.formlyEval($scope, expression, val, val, extraLocals);
+         if (asPromise) {
+           return $q.when (formlyUtil.formlyEval($scope, expression, val, val, extraLocals));
+         } else {
+           return formlyUtil.formlyEval($scope, expression, val, val, extraLocals);
+         }
        }

        function getFormlyFieldLikeLocals(field, index) {
kentcdodds commented 9 years ago

Thanks for the issue and the diff. I'm mostly concerned with the comment // Avoid flicker when hideExpression promises resolve to true... I feel like setting field.hide = true would cause a flicker if the field is not hidden and the promise resolves to false...

My inclination is to not do this and instead have you use a work-around for this. Perhaps something like this: https://jsbin.com/pepode/edit?html,js,output

If this is sufficient for you, please close this issue. If not, maybe there's another way. Thanks!

ecgtheow commented 9 years ago

The 'avoid flicker' part isn't a show-stopper for me - I just think it's better if a field uncloaks after a potential short delay rather than displaying in a half-configured state then vanishing.

Not sure if the workaround is usable for me: my real use-case is hideExpression querying the server to see if the current user has permission to view or edit a field, so the hidden state is external and not just dependent on other form fields. I can certainly try to adapt that though if you don't like the rest of the suggested fix.

kentcdodds commented 9 years ago

I see, it seems like your use case is quite specific. I would actually recommend that you use a router like ui-router or ng-router and utilize the resolve functionality to resolve that asynchronous logic into your controller (so your controller isn't even loaded until that request has finished) and then you'll be able to simply assign the result to the hide property making your controller much easier to understand/maintain/etc.

Either way, the flicker would be a problem for anyone who has the hide property being dynamic as the user interacts with the form and I can't do that to the rest of the users.

Something else you could do is put this particular field into its own form hidden behind an ng-if and do the async stuff outside of formly entirely.

Either way, I don't think we'll be adding this to formly. If you need more help, please follow the instructions here: http://help.angular-formly.com

ecgtheow commented 9 years ago

Using a sub-form might work for me, I'll give that a try. Thanks.

I do suggest though that the documentation be updated to state clearly that promises are not supported with hideExpression.

kentcdodds commented 9 years ago

Will do, thanks for the suggestion! And thanks for using formly :-)

ecgtheow commented 9 years ago

Actually, there won't be any flicker for anyone else :)

We know that no-one else is returning promises from hideExpression functions, for the simple reason that it doesn't work. And $q.when () resolves immediately when given non-promise arguments.

I'm still going to try the sub-form workaround, just thought I'd point that out.