Skip to content
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

Extend simplification + EMCAScript5 compatibility #3022

Merged
merged 4 commits into from
Feb 4, 2013

Conversation

bhouston
Copy link
Contributor

@bhouston bhouston commented Feb 2, 2013

This PR contains these changes to THREE.extend:

Examples and unit tests still work.

@paulmillr
Copy link

  1. Are there any browsers that support WebGL, but don’t support Object.keys?
  2. source.hasOwnProperty( prop ) is a bad idea, because consider source = {hasOwnProperty: 'shit'} etc. The better way is ({}).hasOwnProperty.call(source, prop).

@bhouston
Copy link
Contributor Author

bhouston commented Feb 3, 2013

@paulmillr

1 - I can't answer question 1 as I am not an expert here.

2 - Seems to be costly as it requires a temporary and I'd prefer not to handle that case as THREE.extend is only supposed to be used within the context of Three.js and I don't think we will be screwing up our classes as bad as source = {hasOwnProperty: 'shit'}. I think at some point we have to assume people haven't screwed things up completely because being that defensive will result in a much slower library overall. Although you are correct that our function won't handle this.

@paulmillr
Copy link

As for point 2: it’s not expensive at all. See benchmark. Also, every big javascript lib with extend implements it way I noticed.

@bhouston
Copy link
Contributor Author

bhouston commented Feb 3, 2013

Okay. Will fix this as per Jsperf. Thx.

Sent from my phone, sorry for my grammar and terseness.
On Feb 3, 2013 8:16 AM, "Paul Miller" notifications@github.com wrote:

As for point 2: it’s not expensive at all. See benchmarkhttp://jsperf.com/hasownproperty-performance-analysis/2.
Also, every big javascript lib with extend implements it way I noticed.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3022#issuecomment-13046848.

@bhouston
Copy link
Contributor Author

bhouston commented Feb 4, 2013

@paulmillr, I have just incorporated your suggestions for improving THREE.extends but I left in backwards compatibility with browsers that don't yet support Object.keys. The reason is that THREE.js doesn't necessarily have to be used with WebGL, it can be used with CSS, Canvas, or SVG, or nothing at all if you just want to transfer data around. Thus I feel it is unfair to raise the requirements for Three.js here just to save a few lines of code.

Can you @paulmillr review my changes to make sure that it is best practices?

@mrdoob
Copy link
Owner

mrdoob commented Feb 4, 2013

The reason is that THREE.js doesn't necessarily have to be used with WebGL, it can be used with CSS, Canvas, or SVG, or nothing at all if you just want to transfer data around. Thus I feel it is unfair to raise the requirements for Three.js here just to save a few lines of code.

Very true. Thanks for being careful with this :)

@mrdoob mrdoob merged commit 5316912 into mrdoob:dev Feb 4, 2013
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.

3 participants