Change to a class-based implementation

[gabejohnson]

This PR is related to #16.

To better support conversion to/from forks of ESTree (currently targeting Babel) I've modified the implementation to allow for class-based extensibility.

There's a method createChild and a function childConverter that don't smell right. The former is used to create an instance of a subclass in non-overridden methods. The later is a utility that provides a mapper function for converting collections. Any suggestions on alternatives are welcome.

[bakkot]

Sorry for the delay on this; I'm working on getting the whole suite updated to ES2016. I'll try to get this reviewed sometime next week.

[gabejohnson]

No worries @bakkot. Do you think this PR should target an es2016 branch? It wouldn't take much work to update it once https://github.com/shapesecurity/shift-ast-js/tree/es2016 is published

[bakkot]

No, leave it on this one; we'll update it with the rest of the tools.

[gabejohnson]

I know you're busy @bakkot, but have you had a chance to look this over?

[bakkot]

Finally getting to this. Sorry for the delay, @gabejohnson.

Is there a reason you're copying the node's properties to instances of the class, instead of just recursing?

For example:

class ShiftConverter {
  convert(node) {
    if (!node.type) return null;
    let field = `convert${node.type}`;
    if (typeof this[field] === 'function') {
      return this[field](node);
     }
    throw Error(`convert not implemented yet for: ${node.type}`);
   }

  convertAssignmentExpression(node) {
    let binding = this.toBinding(this.convert(node.left)),
        expression = this.toExpression(this.convert(node.right)),
        operator = node.operator;
    if (operator === "=") return new Shift.AssignmentExpression({ binding, expression });
    else return new Shift.CompoundAssignmentExpression({ binding, expression, operator });
  }

  etc
}

Unless I'm missing something this could be subclassed just as easily, and I think is cleaner. It also does away with convertChild.

childConverter can likewise be avoided, I think.

this.directives.map(childConverter(this))

would be

node.directives.map(d => this.convert(d))

etc.

[gabejohnson]

@bakkot You'd have to ask me from 5 months ago. Near as I can tell, I got stuck on the idea of having the instance being a node instead of a converter.

I can fix it up over the weekend.

Any reason not to just make all of the methods static (other than not strictly being necessary)?

[bakkot]

@gabejohnson only reason I can think not to make methods static is that some later subclass might want to keep some state around during the conversion of an AST. I don't really expect this to happen, so I don't have strong opinions either way.

[gabejohnson]

@bakkot I decided to make everything static and then realized there was no reason to use classes in that case.

So I put all of the functions on a converter object. This makes it pretty easy to extend using Object.assign and referencing the original converter's functions instead of using super.convert*.

I branched off of master again for this work. Would you like me to submit a separate pull request?

[michaelficarra]

@gabejohnson If it would completely supercede this PR, you could just force push it here.

[gabejohnson]

@michaelficarra will do. Do you mind if I remove support for Node v0.12?

[gabejohnson]

Nevermind. That would be a breaking change.

[bakkot]

Looks good to me; if I don't see any issues by tonight I'll go ahead and merge it in.

I think we're planning on continuing 0.12 support as long as Node does anyway, so through the end of the year.

[bakkot]

Sorry, one last thing: since this project doesn't actually useObject.assign, I don't think we should be polyfilling it. Consumers can do that if they want to. Will merge if you take out that bit.

[gabejohnson]

My bad. That was a holdover from my previous implementation. Fixed.

[michaelficarra]

Thanks so much and sorry for the long delay @gabejohnson!

[gabejohnson]

@michaelficarra no worries. Glad I could contribute!