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

Problems Uglifying the code #51

Closed
richtera opened this issue Apr 13, 2020 · 9 comments
Closed

Problems Uglifying the code #51

richtera opened this issue Apr 13, 2020 · 9 comments
Labels
backlog Some day I'll get back to this... bug Something isn't working

Comments

@richtera
Copy link

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
Copy link
Owner

YuriGor commented Apr 13, 2020

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
Copy link
Author

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
Copy link
Owner

YuriGor commented Apr 13, 2020

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

@YuriGor YuriGor added invalid This doesn't seem right enhancement New feature or request and removed invalid This doesn't seem right labels Apr 13, 2020
@richtera
Copy link
Author

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
Copy link
Owner

YuriGor commented Apr 13, 2020

Do you use deepdash-es with a webpack?

@richtera
Copy link
Author

richtera commented Apr 13, 2020

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
Copy link
Owner

YuriGor commented Apr 14, 2020

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 YuriGor added bug Something isn't working and removed enhancement New feature or request labels Apr 14, 2020
@YuriGor
Copy link
Owner

YuriGor commented Apr 14, 2020

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
Copy link
Owner

YuriGor commented Apr 16, 2020

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)

@YuriGor YuriGor closed this as completed Apr 16, 2020
@YuriGor YuriGor added the backlog Some day I'll get back to this... label Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Some day I'll get back to this... bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants