Convert System from monkey-patch to class instance.

[johnjbarton]

This will allow downstream code to extend the implementation of System

[guybedford]

I like the idea of moving to a proper class inheritance over monkey patching, but we should do this carefully in a way that can fully align with the spec.

For example, System instanceof Loader would be broken here. That may seem like a small thing, but we should still consider them all.

[johnjbarton]

For reference I will quote the entire spec for System:

26.4 The System Object

The System object is the Loader Object instance associated with the Realm of the current global object.

I added a test for System instanceof Loader, see #181, and verified that this PR passes that test.

Since the phrase 'associated with the Realm' has no technical meaning, and we don't have any support (or even a spec for ) for Realm, I have a hard time seeing how we can be inside or outside the bounds of the spec here.

[guybedford]

I can't see the updated code on this PR, have you committed it to the branch?

So the way I would expect to subclass the System loader would be:

var myCustomLoader = function(options) {
  System.call(this, options);
}
myCustomLoader.prototype = Object.create(System);
myCustomLoader.prototype.constructor = myCustomLoader;
myCustomLoader.prototype.fetch = function() {
  // use the base class hook
  System.xyz
}
myCustomLoader.baseURL = 'xyz';
myCustomLoader.paths = { ... }; 

But I'm still not sure about this actually, since we're not subclassing the System loader, we're polyfilling it.

I understand wanting to separate the parsing system as a subclass, but I'm not sure it makes sense at this level, and surely there are better ways of encapsulation without breaking the contract with the spec?

[johnjbarton]

Sorry I thought I had pushed.

System.call(this, options);

will fail because System is not a function.

myCustomLoader.prototype = Object.create(System);

will create a hazard because System has instance data.

surely there are better ways of encapsulation without breaking the contract with the spec

But there is no specification of System.

[guybedford]

Sorry, yes I suppose the issue is that System isn't a class to begin with.

I suppose I'd be happier if we can either make a decision to differ from the spec, and just say that System should be a class first, so that it can be subclassed, and not a direct loader instance.

Then we can push for that at the spec level. It would make sense because otherwise, yes, we do just have monkey patching, and I wouldn't be happy turning it into a subclass of something for the polyfill when it isn't planned to be in reality.

[guybedford]

So making System and instance of SystemLoader for example.

[guybedford]

I suppose my idea was that if SystemLoader could extend Loader, then we can have all of your parsing logic in the SystemLoader class, as we don't want to be creating these non-spec classes as part of a spec implementation. Effectively SystemLoader would be the RemappingLoader.

If we can't do something like that, I think we just have to stick with monkey patching and accept the ugliness.

[johnjbarton]

Ok I can wrap it on the traceur side.