-
Notifications
You must be signed in to change notification settings - Fork 2k
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
copy properties, not just fields #2902
Conversation
I'm afraid Coffee targets IE6 - see the older tickets |
so, this sort of thing will never go in? I'm more than happy to add code to detect if those methods exist and fallback to the assignment statement if they don't. |
Sadly, that'd create inconsistencies between browser |
How? Object.defineProperties can't be used on IE6 so we wouldn't have properties to worry about there. I suppose there's also the lookupGetter/defineGetter mess that mozilla did a while back, but people seem to have thankfully shifted away from all that to the more standard ES5 properties. Point is, this is just an interop case for code that is already using properties (and therefore they're supported by the browser.) If the fallback case is added the only problem I can imagine this having is a little more overhead than the actual '=' for non-properties. Right now if you're using a ES5+ implementation and you CS-extend a JS defined function with properties, CS stuffs the value returned from the getter into the CS subclass, which is definitely wrong. |
I find @toshok's argument persuasive. It's perfectly reasonable to knowingly introduce inconsistencies by making use of features only available in certain browsers. Perhaps one is writing a Chrome extension, a Safari bookmarklet, or a UIWebView, in which case one needn't worry about Internet Explorer compatibility. For the sake of argument, let's assume that such a module exists (written in JavaScript, to avoid confusion). Now, I decide to write another piece of code which interacts with it, again in a context where IE compatibility is not a requirement. In this scenario, @toshok's proposal seems compelling. |
Cleanup : Sorry, my bad, I was refering to this point :
From what I can read in the mozilla docs, defineGetter existed before getOwnPropertyDescriptor ? (Is that cosidered "nitpicking"?) This is the only problem I could see. If not, I'll reopen and clean up. |
defineGetter did indeed exist prior to getOwnPropertyDescriptor, but it wasn't standardized. getOwnPropertyDescriptor/hasOwnProperty/defineProperty are all ES5 |
I'm pretty sure a lot of stuff IE6 is doing isn't standardized and we still have to support. Now, if there are only around 0.01% people using that firefox version, maybe it doesn't matter. Cleanup :
I do agree, but this has been a reason for refusal several times : |
…rty, Object.getOwnPropertyDescriptor, or Object.prototype.hasOwnProperty
I've added another commit to my branch for the fallback case (just does normal child[key] = parent[key] if any of the property methods aren't present.) Is there some way to reopen this pull request? or should I open a new one? |
Linked #1039 was to answer @davidchambers 's sentence and give history about old-browser compatibility
I'm only asking if there are inconsistencies in case there's a way to define property but not to get the property descriptor (and if the inconsistency is judged "worth it")
Yes, of course |
there is the old mozilla "defineSetter"/"defineGetter"/etc methods, but hopefully the use of them is vanishingly low, particularly in new portable code, as ES5 has been out for a good long while. Also, the __ methods were never standardized. the ones in the pull request are all directly from ES5. The real question, though, is what browsers (if any) support the "define" methods and don't also support the ES5 methods, because the ES5 methods work just fine with accessor properties defined using the "define" methods. i.e. this works in stable node:
I don't know offhand, but I'd wager that the only JS runtimes that ever supported the "define" stuff also now support the ES5 methods. I'll go out on a limb and say that IE6 doesn't support either :) |
|
that number is inflated quite a bit by mozilla regression tests, and there's also some code editor support files for syntax highlighting, etc. That said, I did see a fair number of defineGetter/defineSetter calls in CS code. And at least one occurrence both "let" and "defineGetter" in the same snippet. sigh.. |
I tend to be in favor, simply because CoffeeScript should work with IE6 and ES5 (i.e. modern JS engines). However, the feature detection needs to get a lot uglier: IE8 has both |
I wondered about that - kangax's matrix mentions that If |
by safe of course I mean we just need to add an |
Nope,
Unless one |
Sorry, I meant Object.prototype.hasOwnProperty. If that returns false we never hit either getOwnPropertyDescriptor or defineProperty. On Apr 3, 2013, at 12:27 PM, Marc Häfner [email protected] wrote:
|
The ancient |
yup, just verified with IE8. var a = {}
Object.hasOwnProperty(a, "foo") == false
a.foo = 5;
Object.hasOwnProperty(a, "foo") == false
Object.getOwnPropertyDescriptor(a, "foo") == exception "Object doesn't support this property or method"
Object.defineProperty (a, "foo", {}) == exception "Object doesn't support this property or method" I'll make the necessary changes to the pull request and update it in a little bit. |
You probably meant: var a = {}
Object.prototype.hasOwnProperty.call(a, "foo") == false
a.foo = 5
Object.prototype.hasOwnProperty.call(a, "foo") == true (IE8 as well) |
crap, you're right. ah well, that's pretty terrible, but will address. |
…ty, Object.getOwnPropertyDescriptor, and Object.prototype.hasOwnProperty are defined, as well as if Object.defineProperty or .getOwnPropertyDescriptor succeed for non-DOM objects.
figured it would be nice to write out the expanded version of extends = function(child, parent) {
for (var key in parent) {
if (#{utility 'hasES5Properties'}) {
if (#{utility 'hasProp'}.call(parent, key))
#{utility 'copyProp'}(child, parent, key);
}
else
child[key] = parent[key];
}
// the rest is the same as the existing extends function
function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; }
hasES5Properties = (function () {
if (!#{utility 'defineProp'} || !#{utility 'getProp'} || !#{utility 'hasProp'})
return false; // catch IE6 (and 7?)
try {
#{utility 'defineProp'} ({}, 'test', {}); // will throw in IE8
#{utility 'getProp'} ({foo:5}, 'foo'); // will throw in IE8
return true;
}
catch (e) {
return false;
}
})() |
That's exactly what we've been trying to tell you. What did you think I meant when I said we pretend accessors don't exist? We treat all member access as plain old side-effect-free member accesses. If we didn't do this, nobody would be able to read the compiler's output, and we would be generating more code than necessary for 99.999% of cases. |
Simmer down, please, gentlemen. There's no reason why everyone can't have their cake and eat it too.
Yes. We currently treat accesses as vanilla JavaScript accesses, cache them when possible -- but don't take extreme care to avoid potential different-values-from-the-same-getter or external side effects. This is a feature, not a bug. If you'd like a different, more getter-friendly CoffeeScript compiler, that's totally rad -- go for it. The reason why we try to annotate the source code and make it accessible is so that you can implement this change (and others), and go to town. Since your resulting compiled JS will continue to be interoperable with ordinary JavaScript, and other CoffeeScript, there's no problem at all. Indeed, I bet some folks would find it helpful if you put a coffee+getters+setters link to your fork on the Wiki. |
What I have is an interest in coffeescript being interoperable with code I write and run into in the wild. If that was workable within coffeescript great. A fork is not something I want to do. Especially for a language. It doesn't help anything to say to users "yeah, it's looks just like coffeescript, but you can't use coffeescript." As far as interop between CS and existing JS you're trying to work with, I'd definitely classify it as a bug. I don't know how you can call it a feature when you can't tell at compile time whether or not you'll break, and you choose the position where you're guaranteed to break. This is just sounding more and more like CS is a minefield where you have to know everything about everything you're using with it in order to keep yourself from lack of interop in CS breaking the resulting whole. Really kinda bummed. But yeah, sorry, I'm done. |
I tried in earnest to offer constructive input, and my words were simply distorted. I don't speak for the coffee-script community, only for myself. Tradeoffs are a fact of life. IMO, this is not worth the overhead, as if I needed to say it again. If you want to quit coffee-script because of this, I wish you the best of luck. Personally, I don't think coffee-script is perfect, but i certainly find it to be less tedious than hand coding JS. YMMV Sent from my iPhone On Apr 4, 2013, at 8:36, Chris Toshok [email protected] wrote:
|
This is quite an interesting topic. Let's try to make the discussion constructive. On one hand, i think that eventual getter/setter support in CoffeeScript would be nice. From a previous discussion on this topic, it seems not everyone agrees on what getters should be able to do; i personally feel like the syntax for accessing a property of an object should not change depending on whether that property is directly stored in the object or computed in some other way (and that's why i'm in favour of supporting getters... a similar arguments for setters could be made). Now, about this particular PR, i think the biggest disadvantage is the complexity needed to make inheritance of constructor properties work. It's sad that so many corner cases exist (the IE8 But automatically ditching new ES semantics because they conflict in some way with the current CS implementation doesn't seem like a good idea either. What @jashkenas suggested is a possible solution: just fork CS and have a "CoffeeScript with getters/setters". But for some reason i'm not in favour of such fragmentation. I feel like telling "just have your own fork" to a possible contributor is not the most motivating response. From this tension, i would propose to consider if the CS compiler could implement some "compile targets". So say if your compiler target is ES6, then the compiler can generate code that uses One thing i'm curious about that maybe @michaelficarra can clarify is:
Can you give an example of this please? I'd really like to know if this assumption of property accesses not having side effects is so pervasive among the generated JS, or if it's more like a handful of cases that could be taken care of without making the compiled JS much more complex. |
'Tis a different subject : #1363 |
@epidemian: |
For the curious, @toshok wrote up a lovely and impassioned blog post here, which y'all should read: http://toshokelectric.com/blog/2013/04/04/why-im-ditching-coffeescript/ Not to beat a dead PR, but I'm still a bit fuzzy as to why the particular Instead of having:
You just have this instead:
... where that
Bada-boom. |
@jashkenas Yep. This guy exploded over nothing. The blog post was just as unproductive as this entire thread. He can simply handle inheritance himself and ignore our baked-in model with the helpful |
You guys realize I get the mail for these updates, right? The point isn't that I can't fix it - I'm perfectly capable of fixing it. The point is that I can't fix it for every downstream user who wants to use coffeescript. That's where you come in. That's why I came to you with a fix. The reason I'm switching all of coffeekit away from coffeescript is that I don't want there to be a fork for my purposes. I don't want to tell my users "hey, you can use coffeescript, but wooooe to thee who uses class, for it breaks." The blog post? that wasn't meant to be productive. And I think you need look no further than yourselves for why this thread was unproductive. I came to you with a fix (and would have gone further toward fixing it), and you both acknowledged a broken piece of your own software, refused to call it a problem (feature? really?), refused a fix on the grounds that it didn't fix enough, and then stated that a complete fix wouldn't be accepted either. Jesus. |
and @jashkenas: I can't post-process like that, given that the getter invocation due to __extends was causing crashes due to invalid getter invocations, now could I? |
sigh let's stay away from this kind of tone, please. This is a discussion about code, not a religious war and figthing over something like this makes me sad. |
Come on, @toshok, be creative. @jashkenas was just giving you a quick example. You can obviously make it work. extend = (child, parent) ->
ctor = -> @constructor = child
ctor.prototype = parent.prototype
child.prototype = new ctor
for own key of parent
Object.defineProperty child, key, Object.getOwnPropertyDescriptor parent, key
child |
sorry, @Nami-Doc.. unfortunately you and I were the only ones who actually had a discussion about code.:/ The rest was most definitely (apparently) a religious war. Do you mean string#replace? I can't do that for downstream users. By the time I see the code, it's already been compiled by whatever transpiler the downstream users uses. Or do you mean globally running a contents_of_file.replace ("__extends () { existing impl }", "__extends () { corrected impl }") ? That would fix (in a terrible, terrible way) the initial problem that I came here to fix, yes. But given what was later said about interop with properties and coffeescript's potential caching by ignoring the fact that .accesses might be properties and not fields, and the general unwillingness to acknowledge the problem or accept fixes, I think it's safer for me and my bug report count just to say goodbye. |
@michaelficarra I am convinced you are not reading a single thing I type. |
@michaelficarra: sorry, let me be more explicit. I worry about my users. Their pain is more important to me than my own pain. Given that your version of extend is exactly what I gave you in the pull request, I am quite confident it will work. It will also not fix a damn thing for any of my users. Specifically, it will not fix it if they want to use coffeescript's "class" sugar. How can you guys be suggesting that I write my own version of extends to fix my code when you won't fix yours? I might have had one to many benzos tonight, but this is really rich. |
You know, re-reading the past few comments has me wondering something. You guys ultimately say no to a fix that uses Object.defineProperty when available because CS caches things such that property accessors will break things, and then you both suggest I work around CS's lack of my fix by suggesting.. my fix? So which is it? Is this pull request sufficient for property interop (in which case everything you said previously is BS) or isn't it (in which case you're knowingly trying to send me down a road where things will break later on)? |
This pull request is not sufficient for property interop. This pull request is also undesirable for the vast majority of current (and future) CoffeeScript users. But you're not just any CoffeeScript user, clearly -- as this ticket makes abundantly clear. For your use case, it sounds like this pull request will work fine, which is why I recommend that you use it for your own purposes. CoffeeScript's Every library that wants to do something a little bit different than the internal |
@toshok you should grow up man, you're embarrassing yourself. |
I'm not entirely convinced by the arguments against this being a bug. I think this is / should be a discussion about the semantics of What class creation means in CoffeeScript:
With the extension/re-definition of properties in ES5 step 2 is kind of broken. From the POV of ES5, Why I think fixing this is reasonable:
Disclaimer: I'm not insisting on anything nor do i want in on the drama. My point is rather that ES5 is a fact of life just like IE6 is. (OTOH: So is, among many other quirks, the brokenness of |
Waking up to the devolution of this thread was extremely disappointing for me. Now, full disclosure, I know @toshok in real life and consider him a friend (a tweet from him is what brought me here to begin with). I understand both sides of this issue, but to me as an engineer (and potential coffeescript user), this thread makes me very wary of the community that has been created here. Only barely was the code in question discussed, and multiple times people have said 'this fix isnt enough', but never was why communicated. In addition to that, a lot of context was 'cleaned' up that would go a long way to explaining why @toshok is so frustrated. Yes, it was moved into a gist, but that makes it very difficult to follow and understand and thread with the existing 'discussion' that was kept. Seemingly random people came out of the woodwork to kibitz (which is normal, hell, I did it!), but to watch those people drop down to somewhat ad hominem attacks (Looking at you @humanchimp, telling someone to grow up when they are expressing frustration is not helpful) while not at all contributing productively is never a good sign. Just to give a bit of background, @toshok has been involved in open source software since most of us where in middle school (yes, he is that old). He worked on Lesstif, a very important project that allowed people to create and run software using Motif w/o their onerous licensing terms. He was also involved with Netscape, and worked at Ximian for a long time (working on Evolution and many other things). Now, while this has nothing to do with 'the price of tea in China', it just speaks to the potential value that he can bring to an open source project. Instead of treating this pull request as an opportunity to discuss the issues, reject his solution constructively and gain his considerable skills as a developer and advocate, members of this community decided to instead attack him and cause him to basically rage quit coffeescript entirely. Which is not a good thing long term for coffeescript, as every opensource project could use any contributor, especially one this valuable. Obviously, @toshok isn't innocent in all this, and I know him well enough to say he probably should have chilled out a bit, but the entire situation would have been avoided by some more thoughtful and honest communication at the start instead of random patch bombing and the general assumption that a potential contributor is stupid. Personally, it sucks to see a situation where everyone loses. |
@tberman, agreed. The one thing i'd like to point out is that not everyone acted in such a childish way as it seems. @davidchambers, @marchaefner, and mostly @Nami-Doc's reponses where very civil and on topic. I think this is a case where a vocal minority can make a cummunity look bad. Name-calling someone is totally unacceptable, and mockingly rejecting someone else's work is very bad as well. Not that @toshok didn't overreact too, but most of the frustration could have been avoided with a better response from the community. That being said, it's not like by disregarding this issue the problem will simply go away. As more people start embracing getters/setters and other ES5 features that CS does not play well with, these kind of issues will become more common. So i think a serious discussion about how we're gonna deal with new JS features is necessary. In that regard, i tried to start a discussion here about how is the compiler disregarding possible side-effects on property accesses. If we could list all instances where the compiler transforms something like |
I probably wasn't even born then :p. Impressive history, that's for sure
Yes it; I just cleaned up everything but I linked the gist.
Agreed. It makes me sad, I'd prefer to have everyone enjoy they share of the cake (or /hug like tenderlove).
That could maybe used for an access soak, but doesn't check |
@humanchimp Sorry I deleted your comment, but we have to stop these attacks now. They don't bring anything constructive and just make people hate on one another. I know you're just answering but it's better to stop now ;). |
Yeah, that was uncalled for. @humanchimp you've taken it too far. edit: toned down my own language. |
:) ok |
@Nami-Doc -- I know you've got super powers now, but please do not delete, edit, or revise in any way other people's comments in the tickets. Just leave 'em as they are, for better or for worse. |
Right now if class A extends class B, and there are accessor properties defined on A, the getter is invoked and the returned value set on B, instead of the property being copied.
this patch copies properties using effectively: