Skip to content

Switch to defineProperty; set enumerable flags to be consistent with private/public properties#728

Merged
starwed merged 2 commits into
craftyjs:developfrom
starwed:defineProperty
Feb 13, 2014
Merged

Switch to defineProperty; set enumerable flags to be consistent with private/public properties#728
starwed merged 2 commits into
craftyjs:developfrom
starwed:defineProperty

Conversation

@starwed
Copy link
Copy Markdown
Member

@starwed starwed commented Feb 7, 2014

__defineGetter and company are deprecated; this switches completely to defineProperty, and uses the enumerable flag to fix #722.

I've tested in Firefox, Chrome, and Safari. It still needs testing in IE and mobile browsers.

Currently we use __defineGetter style syntax, but it has [been deprecated](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineGetter?redirectlocale=en-US&redirectslug=JavaScript%2FReference%2FGlobal_Objects%2FObject%2FdefineGetter).

This switches us to using defineProperty, which works in every browser we support.  (IE9+, any non-ancient version of Firefox, etc.)
This sets it so that the "private" 2D properties (`._x`, `._h` and so on) are non-enumerable, while their associated setter/getters are.

Seems to work in Firefox/Chrome/Safari, haven't checked mobile, can't test it in IE.
@mucaho
Copy link
Copy Markdown
Contributor

mucaho commented Feb 8, 2014

👍
You could also remove the enumerability for viewport private properties (for consistency's sake, although it makes no difference)
Another thing to keep in mind is that Crafty.e().setter() also uses Crafty.support.setter, but that gets refactored in #708 , so I will remove that in #708 once it lands (but I'm sure that was your intention anyway :) )

@mucaho
Copy link
Copy Markdown
Contributor

mucaho commented Feb 8, 2014

I was worried that the property flags could decrease the performance, but that is not the case.

@kevinsimper
Copy link
Copy Markdown
Contributor

@starwed
I can test it on IE and mobile, can this be done trough the tests?

What about the #581, it will run tests in IE easy and fast :)

@starwed
Copy link
Copy Markdown
Member Author

starwed commented Feb 8, 2014

Yeah, if anything goes wrong from this it should be pretty catastrophic, so the tests should catch it! :)

I guess also check the fiddle example from 722 -- you should see both a green and a red square.

(581 looks useful.)

@starwed
Copy link
Copy Markdown
Member Author

starwed commented Feb 10, 2014

Seems to work fine on Android in the mobile versions of Firefox and Chrome.

@starwed
Copy link
Copy Markdown
Member Author

starwed commented Feb 12, 2014

@kevinsimper or anyone else check in IE?

@mucaho
Copy link
Copy Markdown
Contributor

mucaho commented Feb 13, 2014

I can confirm it works for IE

@starwed
Copy link
Copy Markdown
Member Author

starwed commented Feb 13, 2014

Thanks! I'll go ahead and merge then.

starwed added a commit that referenced this pull request Feb 13, 2014
Switch to defineProperty; set enumerable flags to be consistent with private/public properties
@starwed starwed merged commit 69b2b5a into craftyjs:develop Feb 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collision hitbox - wrong coordinates - Crafty.e().clone

3 participants