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

Added clear() and all() to reactive-dict #3135

Closed
wants to merge 1 commit into from

Conversation

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Nov 20, 2014

Added a clear() and an all() method to the ReactiveDict.

The second returns a dictionary with all values, and depends on every key (both current and future). Added tests for the two.

A couple of notes:

  1. Could add a .unset(key) and use it in .clear. Haven't needed that yet.
  2. Not sure if unsetting a key should invalidate equals(key, undefined)?
@glasser
Copy link
Member

@glasser glasser commented Jan 26, 2015

Thanks for the feature request! You might be interested in reading our feature request guidelines.


dict.set('foo', 'bar');
Tracker.flush();
test.equal(_.keys(all).length, 1);

This comment has been minimized.

@stubailo

stubailo Feb 25, 2015
Contributor

It seems like this would be better as just test.equal(all, {foo: "bar"}) unless I'm incorrect that equal does a deep comparison?

This comment has been minimized.

@tmeasday

tmeasday Feb 25, 2015
Author Contributor

Good idea! I didn't realise that. Re-pushed.

@tmeasday tmeasday force-pushed the tmeasday:reactive-dict-extensions branch Feb 25, 2015

self.allDeps.changed();

_.each(oldKeys, function(value, key) {

This comment has been minimized.

@stubailo

stubailo Feb 25, 2015
Contributor

I feel like instead of this, we should just make get and equals depend on self.allDeps - otherwise this will fire a ton of unnecessary reruns?

This comment has been minimized.

@tmeasday

tmeasday Feb 25, 2015
Author Contributor

But allDeps changes whenever you set anything. Perhaps another dep (and better names?)

This comment has been minimized.

@stubailo

stubailo Feb 25, 2015
Contributor

Oh, I see. I think I would have anyDeps and allDeps separately maybe?

  • allDeps is for the situation where all of the keys change - everything should listen to it
  • anyDeps is triggered on any change, and only all depends on it

This comment has been minimized.

@tmeasday

tmeasday Feb 26, 2015
Author Contributor

Yeah, that's what I was thinking too.

Actually, is this an unnecessary optimization? clear() is just going to call changed() a bunch of times on different dependencies, which will in turn invalidate a bunch of computations, but nothing will actually happen until the next flush cycle. So in terms of reruns there'll be no difference, won't there?

Happy to make the change if you want.

This comment has been minimized.

@stubailo

stubailo Feb 26, 2015
Contributor

I think you might be correct, but I think this warrants a review from someone who understands Tracker better than I do. @avital, check this out?

This comment has been minimized.

@avital

avital Mar 4, 2015
Contributor

I have not actually reviewed the diff (and I personally still generally believe that Meteor would be better with less APIs and shorter docs [one of the things, say, people like about React]), but the answer to your specific question is -- yes, autoruns will only re-run once on the next flush cycle.

This comment has been minimized.

@stubailo

stubailo Mar 4, 2015
Contributor

Currently, there is no way to do the things that clear() and all() let you do, so this is actually an upgrade in functionality (resetting a reactive dict and being able to iterate over its contents) rather than just API sugar.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Mar 4, 2015

Not sure if unsetting a key should invalidate equals(key, undefined)

I think it should - for a regular JavaScript object, not having a key is the same as having a key that is set to undefined as far as I know.

val = dict.get('foo');
});
Tracker.autorun(function() {
equals = dict.equals('foo', 'bar');

This comment has been minimized.

@stubailo

stubailo Mar 11, 2015
Contributor

Let's add a test for undefined?

This comment has been minimized.

@tmeasday

tmeasday Mar 11, 2015
Author Contributor

Done, check it out.

@tmeasday tmeasday force-pushed the tmeasday:reactive-dict-extensions branch to 60dd280 Mar 11, 2015
@stubailo
Copy link
Contributor

@stubailo stubailo commented Mar 12, 2015

@debergalis: @avital has expressed some concerns with the API of ReactiveDict/Session growing too much, but these methods actually allow you to do things you couldn't before and often want to do.

Speaking of which: We don't document ReactiveDict anywhere other than Session, so maybe we should start?

@debergalis
Copy link
Member

@debergalis debergalis commented Mar 12, 2015

What's the use case for all and clear? They're not that valuable if you know the key names, right?

@stubailo
Copy link
Contributor

@stubailo stubailo commented Mar 12, 2015

@debergalis this is for the case where ReactiveDict is used as a generic data container, not as a proxy for variable storage like how Session has been used.

For example, you might want to store form errors in a ReactiveDict, and be able to either iterate over them or clear all of the errors. Right now, your only option in that case is to use a Minimongo LocalCollection, which seems like massive overkill to just store a dictionary.

tmeasday added a commit that referenced this pull request Apr 2, 2015
@stubailo
Copy link
Contributor

@stubailo stubailo commented Apr 2, 2015

Merged in the commit above.

@stubailo stubailo closed this Apr 2, 2015
@alexcorre
Copy link

@alexcorre alexcorre commented Jun 9, 2015

@stubailo @tmeasday was this merged in? The api is not available in the latest Meteor @ 1.1.0.2. I don't see a reactive-dict package version bump...perhaps thats the reason?

@stubailo
Copy link
Contributor

@stubailo stubailo commented Jun 9, 2015

We haven't done a release since this was merged :] we probably should soon!

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

Successfully merging this pull request may close these issues.

None yet

6 participants