From 970f1fa7dd9569c53a9f707fa4abaf7c222ec710 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 10 Nov 2017 10:14:41 -0800 Subject: [PATCH 1/9] Some of the most obvious underscore removals --- packages/check/match.js | 55 ++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index e90ea56915..1884e203dd 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -51,8 +51,8 @@ var Match = exports.Match = { Maybe: function (pattern) { return new Maybe(pattern); }, - OneOf: function (/*arguments*/) { - return new OneOf(_.toArray(arguments)); + OneOf: function (...args) { + return new OneOf(args); }, Any: ['__any__'], Where: function (condition) { @@ -242,7 +242,7 @@ var testSubtree = function (value, pattern) { path: "" }; } - if (!_.isArray(value) && !_.isArguments(value)) { + if (!Array.isArray(value) && !_.isArguments(value)) { return { message: "Expected array, got " + stringForErrorMessage(value), path: "" @@ -409,7 +409,7 @@ var testSubtree = function (value, pattern) { } } - var keys = _.keys(requiredPatterns); + var keys = Object.keys(requiredPatterns); if (keys.length) { return { message: "Missing key '" + keys[0] + "'", @@ -418,51 +418,54 @@ var testSubtree = function (value, pattern) { } }; -var ArgumentChecker = function (args, description) { - var self = this; - // Make a SHALLOW copy of the arguments. (We'll be doing identity checks - // against its contents.) - self.args = _.clone(args); - // Since the common case will be to check arguments in order, and we splice - // out arguments when we check them, make it so we splice out from the end - // rather than the beginning. - self.args.reverse(); - self.description = description; -}; +class ArgumentChecker { + constructor (args, description) { + var self = this; + // Make a SHALLOW copy of the arguments. (We'll be doing identity checks + // against its contents.) + self.args = [...args]; + // Since the common case will be to check arguments in order, and we splice + // out arguments when we check them, make it so we splice out from the end + // rather than the beginning. + self.args.reverse(); + self.description = description; + } -_.extend(ArgumentChecker.prototype, { - checking: function (value) { + checking(value) { var self = this; if (self._checkingOneValue(value)) return; // Allow check(arguments, [String]) or check(arguments.slice(1), [String]) // or check([foo, bar], [String]) to count... but only if value wasn't // itself an argument. - if (_.isArray(value) || _.isArguments(value)) { - _.each(value, _.bind(self._checkingOneValue, self)); + if (Array.isArray(value) || _.isArguments(value)) { + _.each(value, self._checkingOneValue.bind(self)); } - }, - _checkingOneValue: function (value) { + } + + _checkingOneValue(value) { var self = this; for (var i = 0; i < self.args.length; ++i) { // Is this value one of the arguments? (This can have a false positive if // the argument is an interned primitive, but it's still a good enough // check.) // (NaN is not === to itself, so we have to check specially.) - if (value === self.args[i] || (_.isNaN(value) && _.isNaN(self.args[i]))) { + if (value === self.args[i] || + (Number.isNaN(value) && Number.isNaN(self.args[i]))) { self.args.splice(i, 1); return true; } } return false; - }, - throwUnlessAllArgumentsHaveBeenChecked: function () { + } + + throwUnlessAllArgumentsHaveBeenChecked() { var self = this; if (!_.isEmpty(self.args)) throw new Error("Did not check() all arguments during " + self.description); } -}); +} var _jsKeywords = ["do", "if", "in", "for", "let", "new", "try", "var", "case", "else", "enum", "eval", "false", "null", "this", "true", "void", "with", @@ -477,7 +480,7 @@ var _jsKeywords = ["do", "if", "in", "for", "let", "new", "try", "var", "case", var _prependPath = function (key, base) { if ((typeof key) === "number" || key.match(/^[0-9]+$/)) key = "[" + key + "]"; - else if (!key.match(/^[a-z_$][0-9a-z_$]*$/i) || _.contains(_jsKeywords, key)) + else if (!key.match(/^[a-z_$][0-9a-z_$]*$/i) || _jsKeywords.includes(key)) key = JSON.stringify([key]); if (base && base[0] !== "[") From a9cd3a491e31243821dcc4f77517f650ebb80239 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 10 Nov 2017 10:17:54 -0800 Subject: [PATCH 2/9] Replace _.isArguments --- packages/check/match.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index 1884e203dd..a0d56ac42d 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -242,7 +242,7 @@ var testSubtree = function (value, pattern) { path: "" }; } - if (!Array.isArray(value) && !_.isArguments(value)) { + if (!Array.isArray(value) && !isArguments(value)) { return { message: "Expected array, got " + stringForErrorMessage(value), path: "" @@ -438,7 +438,7 @@ class ArgumentChecker { // Allow check(arguments, [String]) or check(arguments.slice(1), [String]) // or check([foo, bar], [String]) to count... but only if value wasn't // itself an argument. - if (Array.isArray(value) || _.isArguments(value)) { + if (Array.isArray(value) || isArguments(value)) { _.each(value, self._checkingOneValue.bind(self)); } } @@ -488,3 +488,6 @@ var _prependPath = function (key, base) { return key + base; }; +function isArguments(item) { + return Object.prototype.toString.call(item) === '[object Arguments]'; +} From a8f49169924edcc0b4b5879ce41f57947310e45f Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 10 Nov 2017 10:19:17 -0800 Subject: [PATCH 3/9] Remove _.isEmpty --- packages/check/match.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index a0d56ac42d..5f8a2c41bc 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -121,7 +121,7 @@ var Maybe = function (pattern) { }; var OneOf = function (choices) { - if (_.isEmpty(choices)) + if (!choices || choices.length === 0) throw new Error("Must provide at least one choice to Match.OneOf"); this.choices = choices; }; @@ -461,7 +461,7 @@ class ArgumentChecker { throwUnlessAllArgumentsHaveBeenChecked() { var self = this; - if (!_.isEmpty(self.args)) + if (self.args.length > 0) throw new Error("Did not check() all arguments during " + self.description); } From c18ff138c9a07706ca4d4d174f14be408fbe000a Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 10 Nov 2017 11:27:02 -0800 Subject: [PATCH 4/9] Replace _.has --- packages/check/match.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index 5f8a2c41bc..757cc338f1 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -379,14 +379,14 @@ var testSubtree = function (value, pattern) { for (var keys = allKeys(value), i = 0, length = keys.length; i < length; i++) { var key = keys[i]; var subValue = value[key]; - if (_.has(requiredPatterns, key)) { + if (Object.prototype.hasOwnProperty.call(requiredPatterns, key)) { var result = testSubtree(subValue, requiredPatterns[key]); if (result) { result.path = _prependPath(key, result.path); return result; } delete requiredPatterns[key]; - } else if (_.has(optionalPatterns, key)) { + } else if (Object.prototype.hasOwnProperty.call(optionalPatterns, key)) { var result = testSubtree(subValue, optionalPatterns[key]); if (result) { result.path = _prependPath(key, result.path); From 2f10c82a50dcedc1ffc96a8bef80981fc09ffb69 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 10 Nov 2017 11:27:21 -0800 Subject: [PATCH 5/9] Replace each with forEach --- packages/check/match.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/check/match.js b/packages/check/match.js index 757cc338f1..1e93eec783 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -439,7 +439,7 @@ class ArgumentChecker { // or check([foo, bar], [String]) to count... but only if value wasn't // itself an argument. if (Array.isArray(value) || isArguments(value)) { - _.each(value, self._checkingOneValue.bind(self)); + Array.prototype.forEach.call(value, self._checkingOneValue.bind(self)); } } From f40dac844de131dcfd1cdba011d07dc456d74383 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 10 Nov 2017 11:28:50 -0800 Subject: [PATCH 6/9] Replace _.isObject --- packages/check/match.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index 1e93eec783..55c313da96 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -368,9 +368,9 @@ var testSubtree = function (value, pattern) { }); //XXX: replace with underscore's _.allKeys if Meteor updates underscore to 1.8+ (or lodash) - var allKeys = function(obj){ + function allKeys(obj) { var keys = []; - if (_.isObject(obj)){ + if (obj) { for (var key in obj) keys.push(key); } return keys; From dd336553751e83cbe00f7266f98e007f0fd00fe3 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 13 Nov 2017 22:08:53 -0500 Subject: [PATCH 7/9] Address my own review feedback. --- packages/check/match.js | 74 +++++++++++++++++------------- packages/check/package.js | 3 +- packages/dynamic-import/package.js | 2 +- 3 files changed, 44 insertions(+), 35 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index 55c313da96..f4e002ca9e 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -5,6 +5,7 @@ var currentArgumentChecker = new Meteor.EnvironmentVariable; var isPlainObject = require("./isPlainObject.js").isPlainObject; +var hasOwn = Object.prototype.hasOwnProperty; /** * @summary Check that a value matches a [pattern](#matchpatterns). @@ -45,23 +46,23 @@ var check = exports.check = function (value, pattern) { * @summary The namespace for all Match types and methods. */ var Match = exports.Match = { - Optional: function (pattern) { + Optional(pattern) { return new Optional(pattern); }, - Maybe: function (pattern) { + Maybe(pattern) { return new Maybe(pattern); }, - OneOf: function (...args) { + OneOf(...args) { return new OneOf(args); }, Any: ['__any__'], - Where: function (condition) { + Where(condition) { return new Where(condition); }, - ObjectIncluding: function (pattern) { + ObjectIncluding(pattern) { return new ObjectIncluding(pattern); }, - ObjectWithValues: function (pattern) { + ObjectWithValues(pattern) { return new ObjectWithValues(pattern); }, // Matches only signed 32-bit integers @@ -93,7 +94,7 @@ var Match = exports.Match = { * @param {Any} value The value to check * @param {MatchPattern} pattern The pattern to match `value` against */ - test: function (value, pattern) { + test(value, pattern) { return !testSubtree(value, pattern); }, @@ -101,7 +102,7 @@ var Match = exports.Match = { // `args` (either directly or in the first level of an array), throws an error // (using `description` in the message). // - _failIfArgumentsAreNotAllChecked: function (f, context, args, description) { + _failIfArgumentsAreNotAllChecked(f, context, args, description) { var argChecker = new ArgumentChecker(args, description); var result = currentArgumentChecker.withValue(argChecker, function () { return f.apply(context, args); @@ -360,33 +361,27 @@ var testSubtree = function (value, pattern) { var requiredPatterns = {}; var optionalPatterns = {}; - _.each(pattern, function (subPattern, key) { - if (subPattern instanceof Optional || subPattern instanceof Maybe) + + Object.keys(pattern).forEach(key => { + const subPattern = pattern[key]; + if (subPattern instanceof Optional || + subPattern instanceof Maybe) { optionalPatterns[key] = subPattern.pattern; - else + } else { requiredPatterns[key] = subPattern; + } }); - //XXX: replace with underscore's _.allKeys if Meteor updates underscore to 1.8+ (or lodash) - function allKeys(obj) { - var keys = []; - if (obj) { - for (var key in obj) keys.push(key); - } - return keys; - } - - for (var keys = allKeys(value), i = 0, length = keys.length; i < length; i++) { - var key = keys[i]; + for (var key in Object(value)) { var subValue = value[key]; - if (Object.prototype.hasOwnProperty.call(requiredPatterns, key)) { + if (hasOwn.call(requiredPatterns, key)) { var result = testSubtree(subValue, requiredPatterns[key]); if (result) { result.path = _prependPath(key, result.path); return result; } delete requiredPatterns[key]; - } else if (Object.prototype.hasOwnProperty.call(optionalPatterns, key)) { + } else if (hasOwn.call(optionalPatterns, key)) { var result = testSubtree(subValue, optionalPatterns[key]); if (result) { result.path = _prependPath(key, result.path); @@ -477,17 +472,32 @@ var _jsKeywords = ["do", "if", "in", "for", "let", "new", "try", "var", "case", // Assumes the base of path is already escaped properly // returns key + base -var _prependPath = function (key, base) { - if ((typeof key) === "number" || key.match(/^[0-9]+$/)) +function _prependPath(key, base) { + if ((typeof key) === "number" || key.match(/^[0-9]+$/)) { key = "[" + key + "]"; - else if (!key.match(/^[a-z_$][0-9a-z_$]*$/i) || _jsKeywords.includes(key)) + } else if (!key.match(/^[a-z_$][0-9a-z_$]*$/i) || + _jsKeywords.indexOf(key) >= 0) { key = JSON.stringify([key]); + } - if (base && base[0] !== "[") + if (base && base[0] !== "[") { return key + '.' + base; - return key + base; -}; + } -function isArguments(item) { - return Object.prototype.toString.call(item) === '[object Arguments]'; + return key + base; } + +function isObject(value) { + return typeof value === "object" && value !== null; +} + +function baseIsArguments(item) { + return isObject(item) && + Object.prototype.toString.call(item) === '[object Arguments]'; +} + +var isArguments = baseIsArguments(function() { + return arguments; +}()) ? baseIsArguments : function(value) { + return isObject(value) && typeof value.callee === "function"; +}; diff --git a/packages/check/package.js b/packages/check/package.js index 2d3e057849..c795543a63 100644 --- a/packages/check/package.js +++ b/packages/check/package.js @@ -4,8 +4,7 @@ Package.describe({ }); Package.onUse(function (api) { - api.use('modules'); - api.use('underscore'); + api.use('ecmascript'); api.use('ejson'); api.mainModule('match.js'); diff --git a/packages/dynamic-import/package.js b/packages/dynamic-import/package.js index f23cabe351..5180e2ffb6 100644 --- a/packages/dynamic-import/package.js +++ b/packages/dynamic-import/package.js @@ -15,7 +15,7 @@ Package.onUse(function (api) { api.use("modules"); api.use("promise"); api.use("ddp"); - api.use("check"); + api.use("check", "server"); api.use("ecmascript", "server"); api.mainModule("client.js", "client"); From a30f42c4acabb7591cdde61a1eaeac811610ba3e Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 13 Nov 2017 22:36:43 -0500 Subject: [PATCH 8/9] Use ECMAScript exports instead of CommonJS in check package. --- packages/check/match.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index f4e002ca9e..307502f39b 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -18,7 +18,7 @@ var hasOwn = Object.prototype.hasOwnProperty; * @param {MatchPattern} pattern The pattern to match * `value` against */ -var check = exports.check = function (value, pattern) { +export const check = function (value, pattern) { // Record that check got called, if somebody cared. // // We use getOrNullIfOutsideFiber so that it's OK to call check() @@ -45,7 +45,7 @@ var check = exports.check = function (value, pattern) { * @namespace Match * @summary The namespace for all Match types and methods. */ -var Match = exports.Match = { +export const Match = { Optional(pattern) { return new Optional(pattern); }, From 8a122b7cc2e8d7bd48338171982603637d73c0c8 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Tue, 14 Nov 2017 11:22:31 +0200 Subject: [PATCH 9/9] Finish `check` ES `class` conversion by swapping `self` with `this`. The original PR from @stubailo started the conversion to native ECMAScript `class`es with https://github.com/meteor/meteor/pull/9361/commits/970f1fa7dd9569c53a9f707fa4abaf7c222ec710. I might be projecting, but had there been time to finish that PR, I believe a subsequent commit would have expunged the no-longer-necessary `self=this` assignments, however I think that effort was lost in the hand-off. --- packages/check/match.js | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/packages/check/match.js b/packages/check/match.js index 307502f39b..b387e2d0ee 100644 --- a/packages/check/match.js +++ b/packages/check/match.js @@ -415,39 +415,36 @@ var testSubtree = function (value, pattern) { class ArgumentChecker { constructor (args, description) { - var self = this; // Make a SHALLOW copy of the arguments. (We'll be doing identity checks // against its contents.) - self.args = [...args]; + this.args = [...args]; // Since the common case will be to check arguments in order, and we splice // out arguments when we check them, make it so we splice out from the end // rather than the beginning. - self.args.reverse(); - self.description = description; + this.args.reverse(); + this.description = description; } checking(value) { - var self = this; - if (self._checkingOneValue(value)) + if (this._checkingOneValue(value)) return; // Allow check(arguments, [String]) or check(arguments.slice(1), [String]) // or check([foo, bar], [String]) to count... but only if value wasn't // itself an argument. if (Array.isArray(value) || isArguments(value)) { - Array.prototype.forEach.call(value, self._checkingOneValue.bind(self)); + Array.prototype.forEach.call(value, this._checkingOneValue.bind(this)); } } _checkingOneValue(value) { - var self = this; - for (var i = 0; i < self.args.length; ++i) { + for (var i = 0; i < this.args.length; ++i) { // Is this value one of the arguments? (This can have a false positive if // the argument is an interned primitive, but it's still a good enough // check.) // (NaN is not === to itself, so we have to check specially.) - if (value === self.args[i] || - (Number.isNaN(value) && Number.isNaN(self.args[i]))) { - self.args.splice(i, 1); + if (value === this.args[i] || + (Number.isNaN(value) && Number.isNaN(this.args[i]))) { + this.args.splice(i, 1); return true; } } @@ -455,10 +452,9 @@ class ArgumentChecker { } throwUnlessAllArgumentsHaveBeenChecked() { - var self = this; - if (self.args.length > 0) + if (this.args.length > 0) throw new Error("Did not check() all arguments during " + - self.description); + this.description); } }