YuriGor / deepdash

eachDeep, filterDeep, findDeep, someDeep, omitDeep, pickDeep, keysDeep etc.. Tree traversal library written in Underscore/Lodash fashion
https://deepdash.io/
MIT License
274 stars 12 forks source link

Problems Uglifying the code #51

Closed richtera closed 4 years ago

richtera commented 4 years ago

We are seeing code which are incompatible with "use strict"

ERROR in js/vendors~ui.8f578b013cec78bc290e.js from UglifyJs
In strict mode code, functions can only be declared at top level or immediately within another function. [./node_modules/deepdash/private/getIterate.js:101,8][js/vendors~ui.8f578b013cec78bc290e.js:186542,8]

at deepdash/private/getIterate.js:101

      !options['break'] &&
      res !== false &&
      !isCircular &&
      _.isObject(value)
    ) {
      if (options.childrenPath !== undefined) {
        function forChildren(children, cp, scp) { // DECLARING A FUNCTION WITHIN AN EXPRESSION
          if (children && _.isObject(children)) {
            _.forOwn(children, function(childValue, childKey) {
              var childPath = (path || []).concat( (cp || []), [childKey]);
              var strChildPath =
                options.pathFormat == 'array'
                  ? pathToString([childKey], strPath || '', scp || '')
                  : undefined;
              iterate({
                value: childValue,
                callback: callback,
YuriGor commented 4 years ago

Hi, yeah, my linter also has complained about this... But as you see, I heavily rely on variables from surrounding closure - it would be a pain to extract this function. How do you think, if I'll create it like let forChildren = function(.... will everybody be happy with it?

richtera commented 4 years ago

The only thing I could think of is to put the function at the root of the parent function as a generator to add additional closures on it.

function genForChildren(options, deps, callback, currentObj, currentParents, /* put other closures here */) {
        return function forChildren(children, cp, scp) {
          if (children && _.isObject(children)) {
            _.forOwn(children, function(childValue, childKey) {
              var childPath = (path || []).concat( (cp || []), [childKey]);
              var strChildPath =
                options.pathFormat == 'array'
                  ? pathToString([childKey], strPath || '', scp || '')
                  : undefined;
              iterate({
                value: childValue,
                callback: callback,
                options: options,
                key: childKey,
                path: childPath,
                strPath: strChildPath,
                depth: depth + 1,
                parent: currentObj,
                parents: currentParents,
                obj: obj,
                childrenPath: cp,
                strChildrenPath: scp,
              });
            });
          }
        }
}

and then generate the function when you need it. const fn = genForChildren(...) I am sure there are a lot of different ways to this, but I agree this is something of a pain in the "strict" rule for javascript.

YuriGor commented 4 years ago

Ok, I will see in a few days what can I do (hope it's not show stopper for you)

richtera commented 4 years ago

Currently I had to remove it from my project because I couldn't figure out a way to make it work in my webpack setup :-(

YuriGor commented 4 years ago

Do you use deepdash-es with a webpack?

richtera commented 4 years ago

I have not, I could check. Basically what happened was that our build suddenly broke and I traced it down to this module. I figured it would be good to know on your side and I have looked for a quick fix on my side. Will definitely check the -es version, we were using just "npm install deepdash" but I did not specifically configure webpack to use the es files. Thanks for all your help.

YuriGor commented 4 years ago

I don't know if it will help with uglifier, but probably it should: this 'strict' mode is actually added to cjs build by Rollup, and there is no strict directive in es version. So hope uglifier will take it easier this time.

Another good reason to use es version is theoretically supported tree shaking.

btw I am going to fix this issue, maybe just by instructing rollup to not make things harder or I'll find some time for refactoring

YuriGor commented 4 years ago

Could you, please share your config for uglifyi? I tried here: https://skalman.github.io/UglifyJS-online/ and with default options, it works well If I set strict: true, it fails on trailing comma in a literal object, not on in scope function declaration.

YuriGor commented 4 years ago

Hey, I tried to replace function declaration by assignment it to variable and eslint was satisfied. I don't think it will help in your case but let's try) Another solution for you is to adjust uglifier setting and turn 'strict' option off (does it really brings any benefit?) Anyway, I need to do great refactoring, maybe replace recursion by loop, etc, so I will not try fix this particular issue, but will keep it in mind)