-
Notifications
You must be signed in to change notification settings - Fork 2k
Class system extendability - A simple solution #1762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
One more thing... for ultimate flexibility the |
This doesn't necessarily need to be baked into the language. See https://github.com/jashkenas/coffee-script/wiki/Mixins for a quick and dirty example with similar goals. I do kind of like your suggestion, but I don't really like how it complicates coffeescript's |
@soniciq: we used to have this feature, in the form of the |
@michaelficarra The Mixin example is possible however if your classes need to introspect property definitions etc, there is currently no way to achieve this. I see what you mean about not wanting to complicate things but to be honest I actually think it makes thinks simpler... for the few extra bytes of additional output you get the ability to include instance properties, the ability to extend classes, the ability to extend the class system itself and smaller files when minified. Here is an updated var __hasProp = Object.prototype.hasOwnProperty, __extends = function(child, parent) {
var mixin = function(target, obj) {
for (var key in obj) { if (__hasProp.call(obj, key)) target[key] = obj[key]; } };
mixin(child, parent);
function ctor() { this.constructor = child; }
ctor.prototype = parent.prototype;
child.prototype = new ctor;
child.__super__ = parent.prototype;
child.include = child.include || function(obj) { mixin(this.prototype, obj); };
child.extend = child.extend || function(obj) { mixin(this, obj); };
if (parent.inherited) parent.inherited(child);
return child;
}; Update: Also update original code. |
Obviously a +1 from me :) - this would be awesome! Would also help people compartmentalize and DRY their code by encouraging modules. It's a small change, but could have a big impact on how people write their CoffeScript. |
+1 - seems a good trade-off. |
I think this is a great idea. I think it can be misleading when using the I think if this is done, it would be really cool to add a |
Seems weird to overload '@' to mean class, In all other usages I thought it meant 'this', i.e. references the instance? What about 'class.' or the name of the Constructor i.e. Foo?
|
@mythz: that has nothing to do with this issue. The context of a class body is the class itself, not the instance. I don't particularly agree with it, but it's the way it is. The class name can also be used, as you suggested. This issue proposes adding the Now that I think about it, classes won't have |
@mythz This is how CoffeeScript works currently, the '@' is a shortcut to 'this.'. @ALL Right, now we have a few +1's, does anyone know how to go about getting this into CoffeeScript. I've had a look at the code with a view to implement the change but it's all scary parser stuff and I'm not really sure where to start... I will continue to investigate but if anyone else wants to chip in, please do. |
@soniciq: @michaelficarra raises a good point, which needs to be addressed before we can go any further. If the class doesn't extend another class, than all that extra super and inheritance code won't be generated - not sure how we get round that. |
@michaelficarra and @maccman Sorry missed your comment while writing my last one. That is a good point, I think I have a solution though... give me 15 to whip it up. |
Right, here is a basic implementation, I'm working on cleaning it up a little. Note that the For the CoffeeScript: class Foo
myInstanceMethod: -> console.log 'Instance method'
@myClassMethod: -> console.log 'Class method'
class Bar extends Foo
myInstanceMethod: -> console.log 'Instance method'
@myClassMethod: -> console.log 'Class method' The following would be generated (you can copy this into a js console and var Bar, Foo;
var __hasProp = Object.prototype.hasOwnProperty,
__mix = function(target, obj) {
for (var key in obj) { if (__hasProp.call(obj, key)) target[key] = obj[key]; } },
__extends = function(child, parent) {
__mix(child, parent);
function ctor() { this.constructor = child; }
ctor.prototype = parent.prototype;
child.prototype = new ctor;
child.__super__ = parent.prototype;
if (parent.inherited) parent.inherited(child);
return child;
};
Foo = (function() {
function Foo() {}
Foo.include = function(obj) { __mix(this.prototype, obj); };
Foo.extend = function(obj) { __mix(this, obj); };
Foo.include({
myInstanceMethod: function() {
return console.log('Instance method');
}
})
Foo.extend({
myClassMethod: function() {
return console.log('Class method');
}
});
return Foo;
})();
Bar = (function() {
__extends(Bar, Foo);
function Bar() {
Bar.__super__.constructor.apply(this, arguments);
}
Bar.include({
myInstanceMethod: function() {
return console.log('Instance method');
}
})
Bar.extend({
myClassMethod: function() {
return console.log('Class method');
}
});
return Bar;
})(); |
A little problem: class Foo
@include: ->
oops: -> You don't need to compile to |
@michaelficarra compiling to |
How about this: https://gist.github.com/1276838 |
+1 for more investigation and discussion... I'm new to CoffeeScript - but I like the idea of modularising code. The resulting JS is pretty epic - but I suppose when you're trying to do a Class based style your going to get pretty complex JS output. Good luck! |
@soniciq: I thought the proposal was to only use class Foo
@include someObject # just a regular method call
someMethod: -> # define using `Foo.prototype.someMethod =` I don't see what the benefit would be of passing class Foo
@include: ->
notGoingToBeDefined: -> |
FWIW you can kind of roll your own classes in CS already:
If you want to experiment with class extension mechanisms without jumping right into the CS parser itself, the above code might be useful. |
@michaelficarra I envisaged class Foo
@include: (obj) ->
console.log "Intercepted '#{key}' and did something clever" for key in obj
super
class Bar extends Foo
myProp: 20 This would output: As for the problem case, I don't really see it as a problem as long as it's documented what My Gist is currently work in progress and won't take into account the above 'super' call, I think the best way to candle this is with a super simple base constructor which I am looking into. |
Right, I think I have it guys. This new Gist implements a super-super-simple base class which makes everything just work including the code in my previous comment: https://gist.github.com/1276838 |
@showell I was hoping to get something committed to CoffeScript to save having to jump through hoops like this. |
@soniciq: I like it. It's a much better solution. As to whether it should be included with CoffeeScript, I'm neutral. I don't see myself using the mixin pattern much, so if anything, I would default to keeping the compilation simpler. But the |
Here's a jsFiddle if anyone wants a play: http://jsfiddle.net/A7PxS/ I am convinced this is the way to go, the added benefit you get from the additional 5 or 6 lines is immense, besides, by the time you have more than a couple of instance methods in your class, the total file size will be less than without this addition anyway as there's not |
Thx for the fiddle - excellent work from what I can see/understand. Some less technical feedback.... before jumping to "I am convinced" take some time away from your desk. Stop thinking about the problem/solution. Then come back with a clear head and re-evaluate your work... Sounds a bit redundant, but I know when I "sleep on it" I general change my opinion the next day. Just a thought. Again, great work. |
@chrisjacob You are right, I was getting a little obsessive about this... I have slept on it now :) I decided to think about other use-cases to give some perspective. Correct me if I'm wrong but I think this also helps with #1604 as you could just override I did think about attacking this at a slightly lower level, for example having Foo.defineProtoProperty('myInstanceMethod', function() {
return 'I am an instance method';
});
Foo.defineClassProperty('myClassMethod', function() {
return 'I am an class method';
}); This would again use a super simple base class like the one in the previous Gist and would make it really easy to extend the class system. The method names I've used here are probably too verbose but I've used them so it is clear what is going on. Full implementation here: https://gist.github.com/1278710 What are peoples thoughts on the two options? Just chip in if I'm heading off course: Option 1: https://gist.github.com/1276838 I think we definitely need an |
+1 Has anyone followed up on this? I'd love to be able to use |
+1 In most of my projects I have wanted to use the elegant class syntax but with a few extra hooks or custom features. The needs vary by project, but most of them could be fulfilled by these proposals. |
@soniciq |
Closing since the extended hook has been removed. |
Hi,
The one thing keeping me from using CoffeeScript full-time is that it's class system cannot be extended in any way. Below is an idea that allows users to hook into the class system, it's very similar to that in Ruby and Spine.js.
I am suggesting that the following coffee script:
Should generate:
This way, should Bar want to hook into the class system it could do the following:
I think this would make CoffeeScript's class system truly powerful and allow things like Ext's preprocessors... the applications are endless. The other benefit of this is that the resulting minified JS would be smaller due to the object notation used in
include
andextend
. Obviously theinclude
andextend
function names are just a suggestion but I think the likeness to Ruby is a good thing. To avoid conflicts with existing code, they could always be__include
and__exclude
.This tiny change also introduces Ruby-esque 'Modules' e.g.
I would create a patch but don't know enough about parsers to be able to implement it. I really hope we can add this as currently there is no way to extend the class system.
Thanks,
Jamie
The text was updated successfully, but these errors were encountered: