dthree / vorpal

Node's framework for interactive CLIs
http://vorpal.js.org
MIT License
5.64k stars 280 forks source link

Bug - seems like a legit bug to me #251

Closed ORESoftware closed 6 years ago

ORESoftware commented 7 years ago

I am on "version": "1.12.0"

I got this error:

 TypeError: Cannot read property 'prototype' of undefined
    at _loop (/Users/alexamil/WebstormProjects/oresoftware/sumanjs/suman-d/node_modules/vorpal/dist/ui.js:123:42)
    at new UI (/Users/alexamil/WebstormProjects/oresoftware/sumanjs/suman-d/node_modules/vorpal/dist/ui.js:150:7)
    at Object.<anonymous> (/Users/alexamil/WebstormProjects/oresoftware/sumanjs/suman-d/node_modules/vorpal/dist/ui.js:595:10)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19) 

I was using this code from the example in the readme:

  const vorpal = require('vorpal')();

  vorpal
    .command('foo', 'Outputs "bar".')
    .action(function (args: Array<string>, cb: Function) {
      this.log('bar');
      cb();
    });

  vorpal
    .delimiter('suman>')
    .show();
ORESoftware commented 7 years ago

I have migrated most of my JS library projects to TS and I would recommend that a lot, it can catch these kinds of bugs. If you need help migrating this library to TS, I would definitely volunteer.

ORESoftware commented 7 years ago

I am really looking forward to using this library, so hopefully we can square this bug away.

I can confirm the bug occurs on the very first line:

const vorpal = require('vorpal')();

milesj commented 7 years ago

@ORESoftware What version are you using?

What happens if you set a const first.

const Vorpal = require('vorpal');
const vorpal = new Vorpal();
ORESoftware commented 7 years ago

@milesj I tried that first actually, got the same problem

I am on Vorpal "version": "1.12.0", Node.js version 6.10.0

ORESoftware commented 7 years ago

It might have been that I used

import * as Vorpal from 'vorpal';

I forgot I had that at the top of my file

then later I called

const Vorpal = require('vorpal');

but something is wrong if you get that TypeError.

Suddenly it's working but honestly not sure why. Please keep this issue open, I might have more info for you.

ORESoftware commented 7 years ago

Ok I figured it out! I was monkey-patching some prototypes, I caused the problem, but I am not sure why yet.

Something about the following code causes the problem. If you have any insights please let me know.

'use strict';

const then = Promise.prototype.then;
Promise.prototype.then = function (fn1, fn2) {

  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
    fn2 = fn2 && process.domain.bind(fn2);
  }

  return then.call(this, fn1, fn2);
};

const katch = Promise.prototype.catch;
Promise.prototype.catch = function (fn1) {
  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
  }
  return katch.call(this, fn1);
};

// add a MF utility method to Number.prototype and String.prototype
String.prototype.times = Number.prototype.times = function (callback) {
  if (typeof callback !== 'function') {
    throw new TypeError('Callback is not a function');
  } else if (isNaN(parseInt(Number(this.valueOf())))) {
    throw new TypeError('Object/value is not a valid number');
  }
  for (let i = 0; i < Number(this.valueOf()); i++) {
    callback(i);
  }
};

const exit = process.exit;
let callable = true;

process.exit = function (code, fn) {

  if(!callable){
    if(fn === true){
      exit.call(process, code);
    }
    else{
      console.error(' => Suman implementation warning => process.exit() already called.');
    }
    return;
  }
  else{
    callable = false;
  }

  if (fn) {
    if (typeof fn !== 'function') {
      throw new Error(' => Suman internal implementation error => Please report ASAP, thanks.');
    }
    setTimeout(function () {
      console.error(' => Function timeout.');
      exit.call(process, (code || 1));
    }, 15000);

    let final = function (err, c) {
      if (err) {
        exit.call(process, (c || code || 1));
      }
      else {
        exit.call(process, (c || code || 0));
      }
    }

    let isFinalCallable = true;

    fn(function(){
      if(!isFinalCallable){
        console.error(' => Suman implementation warning => process.exit() already called.');
        return;
      }
      isFinalCallable = false;
      final.apply(null, arguments);
    });
  }
  else {
    exit.call(process, code);
  }
};

Function.prototype.adhere = function () {

  let self = this;
  let args1 = Array.from(arguments);

  return function () {

    let args2 = Array.from(arguments);
    return self.apply(this, args1.concat(args2));
  }

};

Array.prototype.mapToObject = function (fn) {

  let obj = {};

  for (let i = 0; i < this.length; i++) {

    let ret;

    if (fn) {
      ret = fn.call(this, this[i], i);
    }
    else {
      ret = this[i];
    }

    let keys = Object.keys(ret);
    assert(keys.length, ' => Object needs keys.');

    for (let j = 0; j < keys.length; j++) {
      const k = keys[j];
      obj[k] = ret[k];
    }

  }

  return obj;

};

I am going to try to figure out which one of the patches causes the problem. Will report back. They say don't monkey patch prototypes lol...

ORESoftware commented 7 years ago

Ok found it..can you guess which one caused the problem?

It was the Array.prototype.mapToObject patch.

Do you have any idea why? Perhaps you guys made the same patch?

If that's the case...maybe neither of us should do that lol.

milesj commented 7 years ago

This is generally why modifying the prototype is frowned upon. It causes far too many issues across all libraries. That being said, I'm still not sure why modifying the Array caused the problem.

ORESoftware commented 7 years ago

Yeah me neither, pretty weird. I'd like to blame vorpal...I haven't looked at your source where the error is but I will tomorrow. It helps to be precise with your language..you mean Array.prototype.

dthree commented 7 years ago

I definitely don't extend natives in Vorpal.