Refactoring some tests slightly and implementing a deprecation for #390

Author: Kent C. Dodds

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 6.22.1
+
+## Deprecations
+
+- You will now get a deprecation warning if you specify the `skipNgModelAttrsManipulator` property under the `data` property (it has moved to `extras`). [#390](/../../issues/390)
+
 # 6.22.0
 
 ## New Features

other/ERRORS_AND_WARNINGS.md

@@ -142,6 +142,32 @@ the apiCheck checkers are never even created. Not much we can do about the coupl
 a big issue. For more info, see [#334](https://github.com/formly-js/angular-formly/issues/334). Note, this will be
 removed in a major release.
 
+# skipNgModelAttrsManipulator moved
+
+This property has been moved from the `data` property to the `extras` property.
+
+Before:
+
+```javascript
+{
+  template: '<hr />',
+  data: {
+    skipNgModelAttrsManipulator: true
+  }
+}
+```
+
+After:
+
+```javascript
+{
+  template: '<hr />',
+  extras: {
+    skipNgModelAttrsManipulator: true
+  }
+}
+```
+
 # Notes
 
 It is recommended to disable warnings in production using `formlyConfigProvider.disableWarnings = true`. Note: This will

src/directives/formly-custom-validation.js

@@ -44,13 +44,15 @@ function formlyCustomValidation(formlyConfig, formlyUtil, $q, formlyWarn) {
         const isPossiblyAsync = !angular.isString(validator);
         let validatorCollection = (isPossiblyAsync || isAsync) ? '$asyncValidators' : '$validators';
 
+        // UPDATE IN 7.0.0
         // this is temporary until we can have a breaking change. Allow people to get the wins of the explicitAsync api
         if (formlyConfig.extras.explicitAsync && !isAsync) {
           validatorCollection = '$validators';
         }
 
         ctrl[validatorCollection][name] = function evalValidity(modelValue, viewValue) {
           const value = formlyUtil.formlyEval(scope, validator, modelValue, viewValue);
+          // UPDATE IN 7.0.0
           // In the next breaking change, this code should simply return the value
           if (isAsync) {
             return value;
@@ -71,6 +73,7 @@ function formlyCustomValidation(formlyConfig, formlyUtil, $q, formlyWarn) {
         let inFlightValidator;
         ctrl.$parsers.unshift(function evalValidityOfParser(viewValue) {
           const isValid = formlyUtil.formlyEval(scope, validator, ctrl.$modelValue, viewValue);
+          // UPDATE IN 7.0.0
           // In the next breaking change, rather than checking for isPromiseLike, it should just check for isAsync.
 
           if (isAsync || isPromiseLike(isValid)) {

src/directives/formly-custom-validation.test.js

@@ -2,6 +2,10 @@
 import _ from 'lodash';
 import angular from 'angular-fix';
 
+import testUtils from '../test.utils.js';
+
+const {shouldWarnWithLog} = testUtils;
+
 describe(`formly-custom-validation`, function() {
   let $compile, $timeout, $q, scope, $log, formlyConfig;
   const formTemplate = `<form name="myForm">TEMPLATE</form>`;
@@ -104,7 +108,7 @@ describe(`formly-custom-validation`, function() {
           scope.options,
           /validators-returning-promises-should-use-asyncvalidators/
         ];
-        shouldWarn(logArgs, () => {
+        shouldWarnWithLog($log, logArgs, () => {
           validate(() => $q.when(), true);
         });
       });
@@ -164,18 +168,4 @@ describe(`formly-custom-validation`, function() {
     scope.$digest();
     expect(field.$valid).to.eq(pass);
   }
-
-  function shouldWarn(logArgs, test) {
-    /* eslint no-console:0 */
-    test();
-    expect($log.warn.logs, '$log should have only been called once').to.have.length(1);
-    const log = $log.warn.logs[0];
-    _.each(logArgs, (arg, index) => {
-      if (_.isRegExp(arg)) {
-        expect(log[index]).to.match(arg);
-      } else {
-        expect(log[index]).to.equal(arg);
-      }
-    });
-  }
 });

src/directives/formly-field.test.js

@@ -1,13 +1,12 @@
 /* eslint no-shadow:0 */
 /* eslint max-statements:[2, 50] */
 /* eslint max-len:0 */
-/* eslint no-console:0 */
 import angular from 'angular-fix';
 import apiCheck from 'api-check';
 import testUtils from '../test.utils.js';
 import _ from 'lodash';
 
-const {getNewField, input, basicForm, multiNgModelField} = testUtils;
+const {getNewField, input, basicForm, multiNgModelField, shouldWarn, shouldNotWarn} = testUtils;
 
 describe('formly-field', function() {
   /* jshint maxstatements:100 */
@@ -1678,31 +1677,4 @@ describe('formly-field', function() {
   function getNgModelCtrl(index = 0) {
     return angular.element(getFieldNgModelNode(index)).controller('ngModel');
   }
-
-  function shouldWarn(match, test) {
-    const originalWarn = console.warn;
-    let calledArgs;
-    console.warn = function() {
-      calledArgs = arguments;
-    };
-    test();
-    expect(calledArgs, 'expected warning and there was none').to.exist;
-    expect(Array.prototype.join.call(calledArgs, ' ')).to.match(match);
-    console.warn = originalWarn;
-  }
-
-
-  function shouldNotWarn(test) {
-    const originalWarn = console.warn;
-    let calledArgs;
-    console.warn = function() {
-      calledArgs = arguments;
-    };
-    test();
-    if (calledArgs) {
-      console.log(calledArgs);
-      throw new Error('Expected no warning, but there was one', calledArgs);
-    }
-    console.warn = originalWarn;
-  }
 });

src/providers/formlyConfig.test.js

@@ -6,7 +6,7 @@
 import angular from 'angular-fix';
 import testUtils from '../test.utils.js';
 
-const {getNewField, basicForm} = testUtils;
+const {getNewField, basicForm, shouldWarn, shouldNotWarn} = testUtils;
 
 describe('formlyConfig', () => {
   beforeEach(window.module('formly'));
@@ -766,25 +766,4 @@ describe('formlyConfig', () => {
       ]);
     });
   });
-
-  function shouldWarn(match, test) {
-    const originalWarn = console.warn;
-    let calledArgs;
-    console.warn = function() {
-      calledArgs = arguments;
-    };
-    test();
-    expect(calledArgs, 'expected warning and there was none').to.exist;
-    expect(Array.prototype.join.call(calledArgs, ' ')).to.match(match);
-    console.warn = originalWarn;
-  }
-
-  function shouldNotWarn(test) {
-    const originalWarn = console.warn;
-    let callCount = 0;
-    console.warn = () => callCount++;
-    test();
-    expect(callCount).to.equal(0);
-    console.warn = originalWarn;
-  }
 });

src/run/formlyNgModelAttrsManipulator.js

@@ -3,7 +3,7 @@ import angular from 'angular-fix';
 export default addFormlyNgModelAttrsManipulator;
 
 // @ngInject
-function addFormlyNgModelAttrsManipulator(formlyConfig, $interpolate) {
+function addFormlyNgModelAttrsManipulator(formlyConfig, $interpolate, formlyWarn) {
   if (formlyConfig.extras.disableNgModelAttrsManipulator) {
     return;
   }
@@ -12,14 +12,13 @@ function addFormlyNgModelAttrsManipulator(formlyConfig, $interpolate) {
 
   function ngModelAttrsManipulator(template, options, scope) {
     const node = document.createElement('div');
-    const data = options.data;
-    if (data.skipNgModelAttrsManipulator === true) {
+    const skip = getSkip(options);
+    if (skip === true) {
       return template;
     }
-
     node.innerHTML = template;
 
-    const modelNodes = getNgModelNodes(node, data.skipNgModelAttrsManipulator);
+    const modelNodes = getNgModelNodes(node, skip);
     if (!modelNodes || !modelNodes.length) {
       return template;
     }
@@ -128,6 +127,22 @@ function addFormlyNgModelAttrsManipulator(formlyConfig, $interpolate) {
     return node.querySelectorAll(query);
   }
 
+  function getSkip(options) {
+    // UPDATE IN 7.0.0
+    let skip = options.extras && options.extras.skipNgModelAttrsManipulator;
+    if (!angular.isDefined(skip)) {
+      skip = options.data && options.data.skipNgModelAttrsManipulator;
+      if (angular.isDefined(skip)) {
+        formlyWarn(
+          'skipngmodelattrsmanipulator-moved',
+          'The skipNgModelAttrsManipulator property has been moved from the `data` property to the `extras` property',
+          options
+        );
+      }
+    }
+    return skip;
+  }
+
   function getBuiltInAttributes() {
     const ngModelAttributes = {
       focus: {

src/run/formlyNgModelAttrsManipulator.test.js

@@ -2,35 +2,53 @@
 import angular from 'angular';
 import _ from 'lodash';
 
+import testUtils from '../test.utils.js';
+
+const {shouldWarnWithLog} = testUtils;
+
 describe('formlyNgModelAttrsManipulator', () => {
   beforeEach(window.module('formly'));
 
-  let formlyConfig, manipulator, scope, field, result, resultEl, resultNode;
+  let formlyConfig, manipulator, scope, field, result, resultEl, resultNode, $log;
   const template = '<input ng-model="model[options.key]" />';
 
-  beforeEach(inject((_formlyConfig_, $rootScope) => {
+  beforeEach(inject((_formlyConfig_, $rootScope, _$log_) => {
+    $log = _$log_;
     formlyConfig = _formlyConfig_;
     manipulator = formlyConfig.templateManipulators.preWrapper[0];
     scope = $rootScope.$new();
     scope.id = 'id';
     field = {
+      extras: {},
       data: {},
       validation: {},
       templateOptions: {}
     };
   }));
 
   describe(`skipping`, () => {
-    it(`should allow you to skip the manipulator wholesale for the field`, () => {
+    it(`should give a deprecation warning when using the data property`, () => {
       field.data.skipNgModelAttrsManipulator = true;
+      const logArgs = [
+        'Formly Warning:',
+        'The skipNgModelAttrsManipulator property has been moved from the `data` property to the `extras` property',
+        field,
+        /skipngmodelattrsmanipulator-moved/
+      ];
+      shouldWarnWithLog($log, logArgs, manipulate);
+    });
+
+
+    it(`should allow you to skip the manipulator wholesale for the field`, () => {
+      field.extras.skipNgModelAttrsManipulator = true;
       manipulate();
       expect(result).to.equal(template);
     });
 
     it(`should allow you to specify a selector for specific elements to skip`, () => {
       const className = 'ignored-thing' + _.random(0, 10);
       field.templateOptions.required = true;
-      field.data.skipNgModelAttrsManipulator = `.${className}`;
+      field.extras.skipNgModelAttrsManipulator = `.${className}`;
       manipulate(`
         <div>
           <input class="first-thing" ng-model="model[options.key]" />
@@ -60,7 +78,7 @@ describe('formlyNgModelAttrsManipulator', () => {
 
     it(`should not skip by selector if skipNgModelAttrsManipulator is a boolean value`, () => {
       field.templateOptions.required = true;
-      field.data.skipNgModelAttrsManipulator = false;
+      field.extras.skipNgModelAttrsManipulator = false;
       manipulate(`
         <div>
           <input class="first-thing" ng-model="model[options.key]" />
@@ -76,7 +94,7 @@ describe('formlyNgModelAttrsManipulator', () => {
     it(`should allow you to skip using both the special attribute and the custom selector`, () => {
       const className = 'ignored-thing' + _.random(0, 10);
       field.templateOptions.required = true;
-      field.data.skipNgModelAttrsManipulator = `.${className}`;
+      field.extras.skipNgModelAttrsManipulator = `.${className}`;
       manipulate(`
         <div>
           <input class="first-thing" ng-model="model[options.key]" />

src/test.utils.js

@@ -1,4 +1,5 @@
-import {merge} from 'lodash';
+/* eslint no-console:0 */
+import _ from 'lodash';
 
 let key = 0;
 
@@ -10,9 +11,50 @@ const multiNgModelField = `
 const basicForm = '<formly-form form="theForm" model="model" fields="fields" options="options"></formly-form>';
 
 export default {
-  getNewField, input, multiNgModelField, basicForm
+  getNewField, input, multiNgModelField, basicForm, shouldWarn, shouldNotWarn, shouldWarnWithLog
 };
 
 function getNewField(options) {
-  return merge({template: input, key: key++}, options);
+  return _.merge({template: input, key: key++}, options);
+}
+
+function shouldWarn(match, test) {
+  const originalWarn = console.warn;
+  let calledArgs;
+  console.warn = function() {
+    calledArgs = arguments;
+  };
+  test();
+  expect(calledArgs, 'expected warning and there was none').to.exist;
+  expect(Array.prototype.join.call(calledArgs, ' ')).to.match(match);
+  console.warn = originalWarn;
+}
+
+
+function shouldNotWarn(test) {
+  const originalWarn = console.warn;
+  let calledArgs;
+  console.warn = function() {
+    calledArgs = arguments;
+  };
+  test();
+  if (calledArgs) {
+    console.log(calledArgs);
+    throw new Error('Expected no warning, but there was one', calledArgs);
+  }
+  console.warn = originalWarn;
+}
+
+function shouldWarnWithLog($log, logArgs, test) {
+  /* eslint no-console:0 */
+  test();
+  expect($log.warn.logs, '$log should have only been called once').to.have.length(1);
+  const log = $log.warn.logs[0];
+  _.each(logArgs, (arg, index) => {
+    if (_.isRegExp(arg)) {
+      expect(log[index]).to.match(arg);
+    } else {
+      expect(log[index]).to.equal(arg);
+    }
+  });
 }