Refactor ShellString into a real class

[nfischer]

ShellString is a huge hack right now, and isn't really implemented as a class with a prototype. This means it can't be modified at runtime, which significantly limits what plugins can do. We should refactor it into an extendable class (preferably one which inherits all string methods).

We should also consider a ShellArray class for methods like ls(), to make implementation easier.

This blocks #486

[freitagbr]

So, the issue with creating a class that inherits from String is that none of the methods on String are generic, so it really isn't possible without major hacking:

var util = require('util');
function ShellString(stdout, stderr, code) {
  this._value = '' + stdout;
  Object.defineProperty(this, 'length', {
    get: function () {
      return this._value.length;
    },
  });
  this.stdout = stdout;
  this.stderr = stderr;
  this.code = code;
}
util.inherits(ShellString, String);
ShellString.prototype.toString = ShellString.prototype.valueOf = function () {
  return this._value;
};

The above is basically the minimum viable implementation. This is significantly easier with ES6 semantics though:

class ShellString extends String {
    constructor(stdout, stderr, code) {
        super(stdout);
        this.stdout = stdout;
        this.stderr = stderr;
        this.code = code;
    }
}

Of course, using this method, we permanently lose compatibility with Node v0.12 and under.

[nfischer]

I just tried this out. I saw some errors using the class approach:

// define ShellString class like you did

ShellString.prototype.foo = function () {
  return 'hi';
};

// ...
var s = new ShellString();
s.foo(); // TypeError: s.foo is not a function (**on node v4, not v6**)

Maybe we could do something with your first approach. We could try a for loop over all attributes, and wrap them with getters and setters. Although I'm not sure if s.foo() will first call get on .foo followed by call, or if it would perform call directly on .foo. This should work for all the simple properties though.

[freitagbr]

Btw, to define methods on a class:

class ShellString extends String {
    constructor() {
        ...
    }
    foo() {
        return 'hi';
    }
}

[nfischer]

Can't you also do it via ShellString.prototype? That's how someone would modify the ShellString class anyway (this is an intended part of the plugin API).

[nfischer]

Bumping to v0.9 milestone

[nfischer]

TypeError: s.foo is not a function (on node v4, not v6)

Sounds like this is easier to fix when we drop v4 (#873). Let's postpone until then.

[jimmywarting]

Seems like shelljs are testing node v8 and have a min req on node v8 in pkg.json

I think classes came to node v6 so it would be possible to modernize this lib into using classes, const and var and some other new things... would you be open to me sending some PR that modernizes this a bit?

Also, what are your view on dropping suport for older node versions that have ended it's support?

[nfischer]

would you be open to me sending some PR that modernizes this a bit?

I would be open as long as it runs in node v8 without transpiling. If you're interested in modernizing the code more generally (to use ES6 / ES2015 features everywhere) could you please file a separate issue to track that?

Also, what are your view on dropping suport for older node versions that have ended it's support?

In general, I'm OK with dropping support for unsupported node versions (based on https://nodejs.org/en/about/releases/). I'd like to keep our min version at v8 for the time being just because the last release (0.8.4) supports >=4 and I'd feel uncomfortable jumping straight to >=12 in the next release (v0.9).

[jimmywarting]

jumping from v4 to v8 wouldn't that require a major release?