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

Make options object prototype-inherited #2300

Merged
merged 3 commits into from
Dec 16, 2013
Merged

Make options object prototype-inherited #2300

merged 3 commits into from
Dec 16, 2013

Conversation

jfirebaugh
Copy link
Member

Fixes #2294

if (props.options && proto.options) {
props.options = L.extend({}, proto.options, props.options);
if (proto.options) {
props.options = L.Util.extend(L.Util.create(proto.options), props.options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also reuse L.Util.create above for proto creation, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, f2b34cd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful.

@mourner
Copy link
Member

mourner commented Dec 15, 2013

This is awesome, thanks a lot for taking on this!

@mourner
Copy link
Member

mourner commented Dec 15, 2013

BTW, did you look how it affects performance? Like, measure instantiating 100000 markers with some options?

@mourner
Copy link
Member

mourner commented Dec 15, 2013

Also, I guess accessing properties that got inherited will become a bit slower because of prototype chain lookup, but it would be insignificant in theory.

expect(o.options).not.to.equal(opts);
});

it("doesn't create a distinct options object if object already has own options", function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a real use case for this? Not doing the hasOwnProperty check makes it a little faster.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that if L.Util.setOptions was called multiple times on the same object you wouldn't want it to add another link in the prototype chain each time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah that makes sense! So all the setStyle stuff would be affected, etc.

@jfirebaugh
Copy link
Member Author

No, I didn't do any performance testing.

mourner added a commit that referenced this pull request Dec 16, 2013
Make options object prototype-inherited
@mourner mourner merged commit a78dc74 into master Dec 16, 2013
@mourner mourner deleted the prototypal-options branch December 16, 2013 23:37
@patrickarlt patrickarlt mentioned this pull request Dec 21, 2014
60 tasks
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.

Make options object prototype-inherited
3 participants