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

Why not use JavaScript getters? #21

Closed
JannesMeyer opened this issue Jul 31, 2014 · 11 comments

Comments

Projects
None yet
7 participants
@JannesMeyer
Copy link

commented Jul 31, 2014

I'm not sure if I'm overlooking something or if this is a stupid question, but... Why not use JavaScript getters?

JavaScript objects have support for getters: (works everywhere except on IE8)

Why not use them (optionally) instead of .get() and .getIn()? Especially for server-side code and for applications that aren't focusing IE8 this could be really helpful.

@Tvaroh

This comment has been minimized.

Copy link

commented Jul 31, 2014

@JannesMeyer AFAIK they are terribly slow. I saw a benchmark (can't find the link at the moment) and they were 10 times slower than usual approach.

@leebyron

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2014

There are a few reasons for this, but the primary reason is that every getter needs to be defined on every instance. That means every time you do a set, the new map needs to call __defineGetter__ for every possible key that could be gotten. This would make mutations O(n), and I'd like to avoid that. Another, lesser reason is that an Immutable.Map can possibly have non-string keys.

I do actually take advantage for property getters functions for Immutable.Record since the keys will always be strings and are pre-defined for each record type.

@leebyron leebyron closed this Jul 31, 2014

@JannesMeyer

This comment has been minimized.

Copy link
Author

commented Jul 31, 2014

@Tvaroh At least with latest Chrome, the method-based getters are actually about 50% slower: http://jsperf.com/getter-setter/8

@leebyron Thank you! That actually makes a lot of sense.

Regarding non-string keys:
I wasn't saying that the get() and getIn() API should be replaced. I was just proposing an optional, more convenient way of doing things. Of course there should still be a more general fallback, like with JavaScript dot syntax:

var o = {
  'some-key': 1,
  '[another key]': 2
};

o.some-key; // ReferenceError
o['some-key']; // 1

o.[another key]; // SyntaxError
o['[another key]']; // 2

Almost everybody is using dot syntax to retrieve properties in the cases where it works (i.e. the key being a valid JavaScript name), because it's just more convenient.

I don't have any solution for the O(n) mutations though.

@JannesMeyer

This comment has been minimized.

Copy link
Author

commented Jul 31, 2014

Upon a little bit of futher research I found out, that apparently the JavaScript feature that would enable this kind of feature without impacting performance is called "direct proxies" in ES6. To my knowledge there is no possibility of a polyfill, though. :-(

Edit:
Found V8's and SpiderMonkey's bug reports tracking the implementation:
Issue 1543 - v8 - Implement Proxy proposal
703537 – Implement Harmony direct proxies

Apparently SpiderMonkey almost fully implements direct proxies whereas V8 only implements an old version of the spec behind a flag. Not very useful.

@leebyron

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2014

That's correct @JannesMeyer - I'd love to experiment with Proxy support in the future, especially when ES6 is a little more broadly supported by both node and browsers, but since it can't be polyfilled it would unfortunately be an experimental feature for quite a long time.

@ivan-kleshnin

This comment has been minimized.

Copy link

commented Feb 17, 2015

Unfortunately, without this you can't pass Map values to 3-rd party code.
It breaks all of React mixins, e.g. linkState. Same with List.size instead of List.length...
I thought this library is proved to be compatible with React, but I was wrong.
Perhaps I overestimate the rate of evolution :(
Any forecasts on when this might change?

@idolize

This comment has been minimized.

Copy link

commented May 14, 2015

@ivan-kleshnin

You can use .toJS() for most things, since the whole point of Immutable data structures is that they shouldn't be changed by outside methods.

If you want the two-way data binding from linkState you can use this library I made for the purpose: reactlink-immutable

@leebyron

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2015

Map is API compatible with the ES6 Map, but not JS Object - unfortunately the language does not provide a capability to do that. There's some discussion in the JavaScript technical committee to find a way to provide read API compatibility performantly, but even with a decision made, it will be months or even more than a year from a first browser adopter, and many years until browsers with that feature are widespread to the point that we can rely on such a feature.

Unfortunately that means custom data structures in JavaScript must have a different read API from Object and Array for the foreseeable future.

@ivan-kleshnin

This comment has been minimized.

Copy link

commented May 15, 2015

@idolize

You can use .toJS() for most things, since the whole point of Immutable data structures is that they shouldn't be changed by outside methods. If you want the two-way data binding from linkState you can use this library I made for the purpose: reactlink-immutable

Yeah, I went that road. Then went back. Unfortunately with both ImmutableJS and Mori your datastructures begin to look very messy. The probability of bugs increases, not decreases.
At least for me. So I switched back to native ones.

@leebyron yes unfortunately that's how it is now.

@scottmas

This comment has been minimized.

Copy link

commented Sep 19, 2017

FWIW, Object.defineProperty is now just as fast as getting/setting via methods. https://jsperf.com/getter-setter/8

@Gregoor

This comment has been minimized.

Copy link

commented Feb 10, 2018

@scottmas awesome! @leebyron any interest in a PR adding getter support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.