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

getters and setters for memorize/active/params? Object.freeze by default? #51

Closed
millermedeiros opened this issue Oct 31, 2012 · 5 comments

Comments

@millermedeiros
Copy link
Owner

I kinda regret of using normal properties instead of getter/setters because JS doesn't complain when we try to set a property that doesn't exist - signal.memoize = true; will be extremely hard to spot (missing r).

It could be fixed by "use strict" + Object.freeze() on ES5:

"use strict";

var changed = new Signal();
Object.freeze(changed);

changed.memoize = true; // throw error since `memoize` doesn't exist

I heard that Object.freeze have some perf impact on V8 but that should be fixed soon.

One option would be to freeze the Signal and SignalBinding automatically during the instantiation but that might prevent users from adding custom behavior to a Signal instance (will reduce the flexibility).

My current opinion is that we should leave it as is to keep backwards compatibility and maybe do getters/setters for a v2.0 release.

@drkibitz
Copy link

keep as is. My autocomplete works ;)
I don't know if v8 will fix the perf impact.
And getters and setters introduce and unnecessary function call for most people.
I use events for a lot, including games, which must dispatch quite a few signals at 60 fps. Introducing extra function calls can hurt things quite a big in these situations.

@millermedeiros
Copy link
Owner Author

The getters/setters would be just for these 3 properties:

changed.memorize(true);
changed.active(true);
changed.defaultArguments(123, 456);

They aren't that common tho... just putting the thoughts out there so in the future we remind why it wasn't coded like this and what I was thinking at the time... Outside feedback is great for putting these things in perspective.

@millermedeiros
Copy link
Owner Author

ahh, forgot to say.. another option would be add a "flag" to signals to toggle the behavior, something like a signals.neuroticMode = true; which would make all signals to be frozen during instantiation... no backwards incompatibility and just a way to enhance development... (could be disabled before deploy) would also not cause issues if used in previous versions (it would fail silently - no side effects)

@r4j4h
Copy link

r4j4h commented Feb 13, 2013

Sounds like you are on the right track but just wanted to add another outside perspective's concern for performance hits. :) (Even if v8 ameliorates performance hits that doesn't help other browsers' JS engines)

@millermedeiros
Copy link
Owner Author

closing this in favor of #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants