Skip to content

Conflict With Coffeescript #248

Closed
bitmage opened this Issue Dec 8, 2012 · 12 comments

4 participants

@bitmage
bitmage commented Dec 8, 2012

isObject is causing a conflict with the coffeescript compile. See issue recorded here:

jashkenas/coffeescript#2588

I like using both Sugar and coffeescript, so it would be nice if they played well together. Any thoughts on impact that a change on this side would have?

Currently this is stopping me from upgrading to the latest version. 3.0 seems to work ok, I'm guessing because it's less aggressive with Object.extend(), but I could be wrong on that.

@bitmage bitmage referenced this issue in jashkenas/coffeescript Dec 8, 2012
Closed

Cannot read property 'generated' of undefined #2588

@jussiry
jussiry commented Dec 8, 2012

@bitmage as a work around, you can delete isObject after extending Object.prototype:

Object.extend()
delete Object.prototype.isObject

I'd love to see this resolved too, but assuming one wants to extend Object.prototype, I guess the only real solution would be to fix it on CoffeeScript's side.

@alFReD-NSH

@jussiry @bitmage

You can do Object.restore('isObject')

@jussiry
jussiry commented Dec 8, 2012

Hmm, what does restore function do? It doesn't seem to remove the function from prototype.

@alFReD-NSH

Nevermind. Probably I misread the docs.
It's functionality is documented in "Opting-In Methods" section in the features page of the site.

@andrewplummer
Owner

So where do we stand on this one?

To be clear, Object.extend() does modify Object.prototype and is very aggressive and not considered safe for that reason. Although I do think that a maximally robust program will make no assumptions and deal with the case of Object.prototype being extended, I would never expect it to, at least not for Object itself (other natives are a different matter).

Anyway that's the problem you're having. Now without delving deeper into the bug itself, restore is intended as a failsafe. When Sugar extends prototypes (typically not Object, but that applies here as well) it holds onto a reference of functions that it overwrites and is able to return them to their original. However it does not delete the methods from the prototype if they were previously undefined, which is what is causing the bug here.

So it seems to me that restore could be setting the prototype to its previous value regardless of what it is (even undefined) and that would solve this problem.

I'm willing to make this change, although remember that Object.extend() is still not going to be considered kosher going forward.

@andrewplummer
Owner

@bitmage Bringing the conversation back over to here... No there's no way to specify which methods are mapped at the moment... although if restore was able to remove or set to undefined then I suppose you could do the inverse, by restoring methods that were added/overwritten....

@andrewplummer
Owner

Another thing to note... Sugar doesn't overwrite methods if they exist (for the most part, there are a couple exceptions) so restore is actually a bit useless without the ability to remove the methods as if the methods DO exist they shouldn't be overwritten anyway, so that's a bit relevant here...

@jussiry
jussiry commented Dec 10, 2012

Thanks for clarifying your point of view @andrewplummer. Making restore remove previously undefined functions sounds like a good idea.

@andrewplummer
Owner

Yeah I have to agree... it makes the most sense in this case...

@andrewplummer
Owner

okay this prompted a rather large change.

restore is now sugarRestore and I added a new method sugarRevert.

Both of these gained the "sugar" prefix as they are very specific to Sugar itself. Notably:

sugarRestore will restore Sugar methods
sugarRevert will restore methods to their previous state before Sugar methods were added.

Now in normal cases sugarRevert is rather useless as Sugar will not override methods that are already defined anyway, but there are 2 scenarios where this method may be useful:

  1. When specifically forcing a method to be overwritten (this can be accomplished with extend) then wanting to revert it back.

  2. When trying to remove the method. In this case since the method did not exist before, Sugar went ahead and added it. This is the scenario relevant to this issue -- in this case calling Object.sugarRevert('isObject') would remove that object from Object.prototype and the entire issue with Coffeescript would be avoided. Now of course bugs like these may be very difficult to find in the first place, but that's one of the many reasons Object.extend() is use-at-your-own-risk. The finding of the bugs I will leave to you (although I'll make an effort to document them like this when found).

@andrewplummer
Owner

ok, this fix is now out in 1.3.8!

@bitmage
bitmage commented Apr 10, 2013

Hey, thanks for the fix and comments. I was able to resolve the issue as stated.

Just popping in here to document another conflict. I found that Object.extend() would occlude the stack trace from errors, but only when I am using CoffeeScript. I narrowed it down to the 'watch' function by process of elimination, but I haven't looked at the coffee/sugar source to determine why.

You can reproduce with this code:

require 'sugar'
Object.extend()
throw new Error 'oops'

And this gives you normal exception behavior back:

require 'sugar'
Object.extend()
Object.sugarRevert 'watch'
throw new Error 'oops'

Based on my current knowledge, this code will get you (nearly?) complete compatibility with coffeescript:

require 'sugar'
Object.extend()
for prop in ['size', 'isObject', 'watch']
  Object.sugarRevert prop

No changes needed, I just wanted to document this.

@bitmage bitmage referenced this issue in Automattic/mongoose May 23, 2013
Merged

Added a clearer error message #1509

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.