Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load only those lodash functions as necessary #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mhamann
Copy link

@mhamann mhamann commented Nov 16, 2017

Helps a good bit with overall package size.

@whitlockjc
Copy link
Owner

I use to do this and I think I removed it out of laziness.

Copy link
Owner

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

I like what you're trying to do, I use to do this, but I think the approach taken makes it hard to swap between _ and a custom version of _ with cherry-picked functions.

@@ -32,7 +32,17 @@
*/

// Cherry-pick lodash components to help with size
var _ = require('lodash');
var _each = require('lodash/each');
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mind stuffing these into an object? The way I use to do it was like this:

var _ = {
  each: require('lodash/each'),
  // ...
}

The reason this is preferred is because I can continue using _ as if I weren't cherry-picking the functions.

@whitlockjc
Copy link
Owner

Also, when you get the code changes in, can you run node ./node_modules/gulp/bin/gulp.js so that the browser binaries are rebuilt?

@mhamann
Copy link
Author

mhamann commented Nov 20, 2017

@whitlockjc ok, no problem. will update the PR shortly...

@whitlockjc
Copy link
Owner

I'm not seeing the package size improvements, unless you're talking about from the NPM perspective. When I build the browser binaries, here are their respective sizes:

  • Require all of lodash *
36      browser/json-refs-min.js
136     browser/json-refs-standalone-min.js
1908    browser/json-refs-standalone.js
320     browser/json-refs.js
  • Cherry-pick lodash *
64      browser/json-refs-min.js
164     browser/json-refs-standalone-min.js
2128    browser/json-refs-standalone.js
540     browser/json-refs.js

Below is the diff of the changes as I would have them in the PR:

diff --git a/index.js b/index.js
index 0265523..f40a263 100644
--- a/index.js
+++ b/index.js
@@ -32,7 +32,18 @@
  */
 
 // Cherry-pick lodash components to help with size
-var _ = require('lodash');
+var _ = {
+  cloneDeep: require('lodash/cloneDeep'),
+  forOwn: require('lodash/forOwn'),
+  isArray: require('lodash/isArray'),
+  isBoolean: require('lodash/isBoolean'),
+  isError: require('lodash/isError'),
+  isFunction: require('lodash/isFunction'),
+  isObject: require('lodash/isObject'),
+  isPlainObject: require('lodash/isPlainObject'),
+  isString: require('lodash/isString'),
+  isUndefined: require('lodash/isUndefined')
+};
 var gl = require('graphlib');
 var path = require('path');
 var PathLoader = require('path-loader');
@@ -58,7 +69,7 @@ function combineQueryParams (qs1, qs2) {
   var combined = {};
 
   function mergeQueryParams (obj) {
-    _.each(obj, function (val, key) {
+    _.forOwn(obj, function (val, key) {
       combined[key] = val;
     });
   }
@@ -326,7 +337,7 @@ function buildRefModel (document, options, metadata) {
     refs = findRefs(document, options);
 
     // Iterate over the references and process
-    _.each(refs, function (refDetails, refPtr) {
+    _.forOwn(refs, function (refDetails, refPtr) {
       var refKey = makeAbsolute(options.location) + refPtr;
       var refdKey = refDetails.refdId = makeAbsolute(isRemote(refDetails) ?
                                                        combineURIs(relativeBase, refDetails.uri) :
@@ -1173,8 +1184,8 @@ function resolveRefs (obj, options) {
       });
 
       // Add edges
-      _.each(results.deps, function (props, node) {
-        _.each(props, function (dep) {
+      _.forOwn(results.deps, function (props, node) {
+        _.forOwn(props, function (dep) {
           depGraph.setEdge(node, dep);
         });
       });
@@ -1191,8 +1202,8 @@ function resolveRefs (obj, options) {
       });
 
       // Process circulars
-      _.each(results.deps, function (props, node) {
-        _.each(props, function (dep, prop) {
+      _.forOwn(results.deps, function (props, node) {
+        _.forOwn(props, function (dep, prop) {
           var isCircular = false;
           var refPtr = node + prop.slice(1);
           var refDetails = results.refs[node + prop.slice(1)];
@@ -1237,13 +1248,13 @@ function resolveRefs (obj, options) {
       });
 
       // Resolve the references in reverse order since the current order is top-down
-      _.each(Object.keys(results.deps).reverse(), function (parentPtr) {
+      _.forOwn(Object.keys(results.deps).reverse(), function (parentPtr) {
         var deps = results.deps[parentPtr];
         var pPtrParts = parentPtr.split('#');
         var pDocument = results.docs[pPtrParts[0]];
         var pPtrPath = pathFromPtr(pPtrParts[1]);
 
-        _.each(deps, function (dep, prop) {
+        _.forOwn(deps, function (dep, prop) {
           var depParts = dep.split('#');
           var dDocument = results.docs[depParts[0]];
           var dPtrPath = pPtrPath.concat(pathFromPtr(prop));
@@ -1329,7 +1340,7 @@ function resolveRefs (obj, options) {
       });
 
       // Sanitize the reference details
-      _.each(results.refs, function (refDetails) {
+      _.forOwn(results.refs, function (refDetails) {
         // Delete the reference id used for dependency tracking and circular identification
         delete refDetails.refdId;
       });

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

Successfully merging this pull request may close these issues.

2 participants