serialport / node-serialport

Access serial ports with JavaScript. Linux, OSX and Windows. Welcome your robotic JavaScript overlords. Better yet, program them!
https://serialport.io
MIT License
5.82k stars 1.01k forks source link

Legacy arguments object pattern causing problem when code is compressed. #1245

Closed rwaldron closed 7 years ago

rwaldron commented 7 years ago

Summary of Problem

When t2-cli compresses JS code with uglify-es to bundle and deploy on Tessel 2, the code is compressed using the ecma: 6 compress option as well as the ecma: 6 output option. Thankfully, this results in very aggressive es6-centric compression, but unfortunately it also reveals "gotchas" of using es6 era features with legacy patterns. There is a problem pattern in lib/util.js, that when compressed, produces this:

(I've obviously run the code through jsbeautifier for readability)

"use strict";
module.exports = {
  promisify: o => {
    if ("function" != typeof o) throw Error('"func" must be a function');
    return () => new Promise((r, t) => {
      const e = Array.prototype.slice.apply(arguments);
      console.log(e[0]), e.push((o, e) => {
        if (o) return t(o);
        r(e)
      }), o.apply(null, e)
    })
  }
};

It may not be obvious at first, but the problem is that there is actually no arguments object in arrow functions:

But uglify-es isn't going to change your original arrow function in the Promise instantiation:

'use strict';

function promisify(func) {
  if (typeof func !== 'function') {
    throw new Error('"func" must be a function');
  }
  return function() {
    return new Promise((resolve, reject) => {
      const args = Array.prototype.slice.apply(arguments);
      args.push((err, data) => {
        if (err) {
          return reject(err);
        }
        resolve(data);
      });
      func.apply(null, args);
    });
  };
}

module.exports = {
  promisify
};

Because it's totally valid to access the arguments object of a function that an arrow function is nested inside of. The great news is that this requires only a minor change to fix! All that needs to be done is for the arguments object to move out of the nested arrow function and into the actual function from which it originated, then uglify-es will know to do the right thing and preserve that function's FunctionExpression syntax.

Patch to follow!

rwaldron commented 7 years ago

Sorry, I forgot to show the source change and corresponding compressed code:

'use strict';

function promisify(func) {
  if (typeof func !== 'function') {
    throw new Error('"func" must be a function');
  }
  return function() {
    const args = Array.from(arguments);
    return new Promise((resolve, reject) => {
      args.push((err, data) => {
        if (err) {
          return reject(err);
        }
        resolve(data);
      });
      func.apply(null, args);
    });
  };
}

module.exports = {
  promisify
};

Results in:

"use strict";
module.exports = {
  promisify: r => {
    if ("function" != typeof r) throw Error('"func" must be a function');
    return function() {
      const n = Array.from(arguments);
      return new Promise((t, o) => {
        n.push((r, n) => {
          if (r) return o(r);
          t(n)
        }), r.apply(null, n)
      })
    }
  }
};

(Again, I ran the code through jsbeautifier for readability)

rwaldron commented 7 years ago

Also, that's the only place in the code that this occurs

reconbot commented 7 years ago

Who would have thunk? You make some good arguments. I promise don't mind merging any patches that slice through this ... aaannd I'm out of puns.

rwaldron commented 7 years ago

😆 🤣

rwaldron commented 7 years ago

Fixed!

mehalshah commented 7 years ago

Alright guys, time to packet in

getify commented 7 years ago

Just curious (side note at best)... why does it produce this:

module.exports = {
  promisify: o => {
...

instead of:

module.exports = {
  promisify(o) {
...