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

Arrays/Objects as Session variables are not reactive #215

Closed
adam-socrenchus opened this Issue Jul 6, 2012 · 29 comments

Comments

Projects
None yet
10 participants
@adam-socrenchus

adam-socrenchus commented Jul 6, 2012

If you store an array or an object in a Session variable, like such:
Session.set('test1', [1,2,3]);
Session.set('test2',{'foo': 0});

None of the following behavior will trigger reactivity:
foo = Session.get('test1');
foo.pop();
Session.get('test1').push(4);
Session.set('test1',foo);

bar = Session.get('test2');
bar.foo = 1;
Session.get('test2')['bar'] = 3;
Session.set('test2',bar);

The only way to trigger reactivity is by setting the Session variable to a different array/object, regardless of content:
Session.set('test1', foo.slice(0));

In session.js, the new value is compared with === against the old value to see if the value has changed and the context should be invalidated, so setting a variable with an updated version of the same object doesn't register as a value change. However, if Session stored a clone of any array/object passed to it, then this wouldn't be a problem; even setting the variable to the same object would fail the check against the stored value, because it would be a clone.

@flisky

This comment has been minimized.

flisky commented Jul 7, 2012

Just FYI, here is the same question in SO.

@audreyr

This comment has been minimized.

Contributor

audreyr commented Aug 17, 2012

When you try to store arrays or objects in Session variables, you should get a warning because you shouldn't be able to do that. This is a bug.

Looks like this isn't in the docs, but it ought to be.

@glasser

This comment has been minimized.

Member

glasser commented Aug 17, 2012

I agree with Audrey that it doesn't work now. Let's think through your suggestion about cloning, though.

The biggest concern with the status quo is that people CAN mutate the value returned by Session.get but it ISN'T reactive. So there are a few directions towards fixing this. Audrey's suggestion is to prevent users from putting mutable objects in Session variables. This means that nobody can even try to mutate the session-stored value, which would work. ie, you CAN'T mutate the values and it ISN'T reactive. This is consistent but also limits the feature.

We could try to make it so that you CAN mutate the values and it is IS reactive. But this seems really hard in general, unless there are some fancy Javascript hooks into value mutation that I don't know about.

But here's something that could work (I think it might even be what you were suggesting?) Let's say that both set and get cloned the value (with a deep copy).

If we did that, then:

Session.set('a', arr);
arr[0] = 1;

would have no effect on Session.get('a'), and neither would

Session.get('a')[0] = 1;

So if we made that change, people could try to mutate the session value, but any testing would reveal that such mutations have no effect on the actual data in Session. So hopefully, people who tested this would realize that you CAN'T mutate the value inside the Session, and thus would expect that it ISN'T reactive.

(To do this change, you'd need to both clone on the way in and out, and then also change the value === old value to compare based on stringification rather than object identity.)

I think this would be a nice compromise between "only scalars in Session values" and "arbitrary mutable values in Session values but mutating them is non-reactive".

@psoots

This comment has been minimized.

psoots commented Sep 1, 2012

Why not add an optional third argument to Session.set(), Session.set(key, value, trigger), that forces the reactivity when set to true?

@psoots

This comment has been minimized.

psoots commented Sep 1, 2012

I submitted a pull request as a possible solution.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 3, 2012

Shouldn't Collections be used to serve as arrays? Or perhaps we need to add an ArrayCollection that supports integer based indexing. Collections can exist on client-side only, too. Just saying, there might be no way to individually track each array element...

@psoots

This comment has been minimized.

psoots commented Sep 4, 2012

@TomWij The problem isn't just limited to using Collections or Arrays or pre-determined objects. Developers also want to store Objects (or Models) in the Session and detecting the change of a regular object seems difficult.

Another alternative would be to have Session.set() always trigger a reaction but add a third argument Session.set(key, alternative, silence) that doesn't trigger the reaction. Several Backbone functions are like this.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 4, 2012

@psoots: I think Documents (and hence collections) fit that purpose perfectly. But, think about your message for a mement... Why would you want objects in Session? Surely that would be considered a misuse of Session.

Session provides a global object on the client that you can use to store an arbitrary set of key-value pairs. Use it to store things like the currently selected item in a list.

There's really no reason to have arrays / objects in Session, feel free to prove me otherwise with an example.


Also note what @audreyr said:

When you try to store arrays or objects in Session variables, you should get a warning because you shouldn't be able to do that. This is a bug.

Looks like this isn't in the docs, but it ought to be.

It's a bug in the documentation that doesn't warn you about it, they explicitly confirm that you shouldn't be able to do that.

If you want to see this change, you'll have to show something that makes a change worth the cost and effort...

@pierredewilde

This comment has been minimized.

pierredewilde commented Sep 4, 2012

Session.set(key, JSON.stringify(obj) may be used as object/array serialization.

JSON.parse(Session.get(key)) may be used as object/array deserialization.

@psoots

This comment has been minimized.

psoots commented Sep 4, 2012

@TomWij Regarding your blockquote from the docs about what a session is: "Use it to store things like the currently selected item in a list." I don't understand why the currently selected item in a list couldn't be an object.

Regarding what @audreyr says: I agree. If you store an array or object in the Session and changing that object's internals does not trigger reactivity, then yes, you should get a warning, and yes, the lack of a warning is a bug. This is because we expect to store something in the Session and trigger reactions when calling Session.set() with a new/updated value. But since the chain reaction doesn't occur, there is a bug.

It's thus very obvious that Session.set() is an incomplete function. And we really have just two options: either throw an error or support Arrays and Objects. (Of course, there are many different ways we can handle the latter option.) And I think the latter option is the most versatile and rewarding for everyone. The cost of not fixing this issue is that people will just find a workaround like what @pierredewilde has suggested and write clumsy, bloated code which is not in tune with Meteor's principles like: "simplicity equals productivity".

We don't have to go with the pull-request that I proposed, but I wanted to propose something because it seems pretty obvious to me that something should be done about this. And adding a third variable (either 'trigger' or 'silence') seems pretty easy and efficient, though not very elegant. However, I have seen other well-liked libraries doing similar things, like Backbone.

However, if we don't want to add a third argument, some sort of serialization of the object would seem essential like what @pierredewilde has suggested but implemented from within the function itself. This is a much more elegant solution, but more costly at run-time and complicated at development time.

@TomWij Are you suggesting that we limit the types of higher-level objects that can be stored in the Session to pre-determined object types? Like Meteor.Collections, etc.? This seems rather intrusive, yeah?

@jagill

This comment has been minimized.

Contributor

jagill commented Sep 4, 2012

@TomWij I also tried to put an Array/Object into the Session, where reactivity would be helpful. To provoke reactivity, I've taken to storing the context of the autosubscribe block in the object, which invalidates it when it is changed meaningfully. Below is my use-case; I'm open to doing it another, more "meteory" way.

We store a priority queue (back by a heap) in the Session, which gets fed deltas by an autosubscribe block that is watching a Model from a Collection. Another autosubscribe block (with Session.get()) watches the priority queue for updates, and then applies them in priority (ie, timestamp) order. This second block is the one that has to be manually invalidated since it can't see the internal change of something being pushed to the queue. We needed to use a priority queue to handle streams of deltas coming from multiple other users, where at times the timestamps were delivered "out of order" (hence the timestamp priority), and we want one queue per session (not user, since a single user might have multiple sessions).

I'm guessing from the thread that this is an abuse of the Session object (despite it being for "arbitrary key-value pairs" :>). What's the proper way to do this with a local collection? Or is this use-case worth supporting?

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 4, 2012

@jagill: You've guessed it right in your last paragraph, I've stopped reading at

We store a priority queue (back by a heap) in the Session, ...

because the rest of that sentence (and even your paragraph, if I do read it) does not explain why you make this particular choice; I don't feel like 1 queue per session justifies that particular choice. Why did you consider the Session for this? What alternatives have you considered?


Reading further, the actual use case / reasoning can be found in the end of that paragraph.

We needed to use a priority queue to handle streams of deltas coming from multiple other users, where at times the timestamps were delivered "out of order" (hence the timestamp priority), and we want one queue per session (not user, since a single user might have multiple sessions).

If I understand that correctly; you want to make sure you apply the deltas in order, this per individual tab and not per browser / computer / user.

Okay, I'm assuming these deltas come from the server, so you're using a combination of the publish and subscribe commands to get the deltas from the source to your client. The first place I would think you want these to end up are in a local Collection on the client. But then you probably exactly get the issue you describe, all the tabs would get the exact same collections.

So, I would solve this using the following approach:

  1. The client first asks a random unique id (for example, a GUID), identifying his session; through a call to a method.
  2. The client stores this unique id in Session.
  3. The client subscribes for the deltas that are meant for his current session (using the unique id).

This last part is perfectly possible with autosubscribe.

This should solve the problem of the client receiving the same things. There's one thing left to solve, the priority. But that should just be a matter of sorting the collection and taking the first element? Or otherwise, it might be appropriate for someone to write a priority queue style Collection if we're talking about scalability up to a real large set of deltas?

Think about it... Deltas are dynamic data that don't identify the current state of a session, but rather a change to it.

This might feel a bit like too much rambling, but that's how I feel the Meteor way would be like.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 4, 2012

@psoots

Regarding your blockquote from the docs about what a session is: "Use it to store things like the currently selected item in a list." I don't understand why the currently selected item in a list couldn't be an object.

Because a selected item in a list would correspond to a document, a document has an id to refer to; so, if you want to maintain reactivity you'd want to insert that id into Session and not insert a non-reactive object that might be subject to change. Then, whenever you need the document it's a trivial find call on its collection; this is all reactive.

And I think the latter option is the most versatile and rewarding for everyone. The cost of not fixing this issue is that people will just find a workaround like what @pierredewilde has suggested and write clumsy, bloated code which is not in tune with Meteor's principles like: "simplicity equals productivity".

Building a system towards exceptions should never be the right approach; we shouldn't be rewarding people for not reading the documentations or ignoring the exceptions or not asking for help early on, it'll add more complexity / effort / cost for no real benefit. Why misuse Session for thing it was not designed for? What will we add next to it? Templates? Hence that's why giving proper examples, that actually show use cases that make sense, a necessity...

@TomWij Are you suggesting that we limit the types of higher-level objects that can be stored in the Session to pre-determined object types? Like Meteor.Collections, etc.? This seems rather intrusive, yeah?

I'm suggesting to not use Session for any of that and just keep it a key / value pair, which is its intended purpose. You would turn a bike into an airplane because the bike misses a few features, right? The bike wasn't designed for that. See my previous message for an example where Session has been misused, where-as this can be done using a fairly simple Key Value Pair in Session as well as a LocalCollection, a server method call and auto-subscribing; and if needed, we could work towards a PriorityQueue based LocalCollection. It's altogether more of a Meteor approach than using Session in a way that it does not store the state of a session, because that would no longer make it a session...

@psoots

This comment has been minimized.

psoots commented Sep 4, 2012

@TomWij I still disagree. But perhaps you should submit a pull-request that throws an error when someone tries to store a non-reactive object in the Session, because in your mind it is an error.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 4, 2012

@psoots: Everything you store in Session is non-reactive. Such a pull request wouldn't make sense...

You might disagree, but those three words alone ain't gonna turn this pull request around.

@psoots

This comment has been minimized.

psoots commented Sep 4, 2012

@TomWij Lets say I have a model that, among other things, maintains the state of my application. Yes, I could store bits and pieces of it in the Session to trigger certain changes in the UI. However, it'd be easier to store the whole model. What would be the Meteor way to do this?

EDIT: The model is a Backbone.Model, if that makes a difference.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 4, 2012

@psoots: Why do you have that model and what do you plan to use it for?

The Meteor way would be to not use a Backbone.Model, as that's meant for events which is the opposite of being reactive; thus the Backbone.Model is simply superseded by Meteor's combination of Collection, Template and Session. Template is where the logic goes, Collection is where the data goes and Session is where the state goes. If you do a comparison, you'll see how extending in Backbone corresponds to defining a function on a Template, how Backbone events are inherited in Meteor's reactivity and how firing a Backbone event is inherited by just changing the data in Meteor instead, whether its in a Document or a Session.

Why would you throw all those components provided by Meteor out of the window? That results in non-reactivity.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 5, 2012

Furthermore, there's meteor-models on atmosphere if you want to maintain the "Model" way to do things, be sure to check out #129 for more details regarding this idea, it has been reopened in #267. Allows you to attach functions to documents...

@psoots

This comment has been minimized.

psoots commented Sep 6, 2012

@TomWij Thanks for working through this with me. I went back through my code and refactored it so that I was just storing strings and things in Session. I probably still have the same number of lines of code. I really like the reactivity of it all and how meteor "just works" sometimes. It just feels a little un-object-oriented at times, though it's not really.

I've closed my pull request.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 6, 2012

Yeah, Full Stack Reactivity is one of the seven principles.

There's been quite some discussion about a possible use case of this on IRC; for instance where you have a "Singleton Document" (as I'd like to call it) representing the state (from an advanced form input), it might not make sense storing this in a Collection but while one would put it in a Session at first. But then you have the problem that it doesn't work reactive, the user in this case just wants to be able to just call the "Singleton Document" itself without having to deal with a Session or a Collection.

While there were quite some suggestions and back-and-forth chatting about the subject, I think that a combination of the forms branch and support for models will already be a good step in the right direction.

tmeasday added a commit to tmeasday/meteor that referenced this issue Sep 27, 2012

Added note warning users about placing arrays/objects in the session.
This is a common gotcha, see for example meteor#215.
I'm not sure if the plan is to eventually support serializable objects in the session, but in the meantime..
@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 27, 2012

Please see my PR to simply point this out for users in the docs.

@glasser

This comment has been minimized.

Member

glasser commented Sep 27, 2012

We'll probably make it clone arrays and objects in and out, but for now I've updated the docs and made Session.set error (like Session.equals does already). Thanks.

@scottburch

This comment has been minimized.

scottburch commented Sep 30, 2012

Making it error is a bad idea. I am using the auth-devel branch because I have patched it (and sent a pull request). Now my working code is broken. I wanted to store objects and understood that changes to the object were not reactive since === would return true. Is there a plan to set this back for people like me, or do I need to rewrite the code?

@jagill

This comment has been minimized.

Contributor

jagill commented Sep 30, 2012

Is the intent that non-reactive, session-based variables (including arrays/objects) are just declared as variables in the JS files and used "normally"?

When I've done that, it seems to work, but scoping with Meteor behaves a little weirdly at times. I'm not quite sure what the Meteor.autocomplete() (and similar) blocks can see. Most of the time the variables declared outside those blocks are included in the closure as normal, but sometimes what I see in log statements and in the browser debugger don't agree.

@glasser

This comment has been minimized.

Member

glasser commented Oct 1, 2012

The current plan is to change Session so that inserting arrays or objects
will clone on get and set (eg, set will JSON-stringify and get will
JSON-parse, and it will track whether or not it did this in a second
dictionary). Only reason I haven't done this yet is that there's a pending
rewrite of Session on the deps-utils branch which I don't want to conflict
with.

On Sat, Sep 29, 2012 at 8:58 PM, Scott Burch notifications@github.comwrote:

Making it error is a bad idea. I am using the auth-devel branch because I
have patched it (and sent a pull request). Now my working code is broken. I
wanted to store objects and understood that changes to the object were not
reactive since === would return true. Is there a plan to set this back for
people like me, or do I need to rewrite the code?


Reply to this email directly or view it on GitHubhttps://github.com//issues/215#issuecomment-9010616.

@glasser

This comment has been minimized.

Member

glasser commented Oct 1, 2012

@jagill This issue isn't the best place for questions that aren't about fixing this particular issue. Try the meteor-core or meteor-talk lists, or IRC, with a more specific question (eg, examples of what seemed strange in a log statement).

Certainly server-side, you should not assume anything about code outside of method definitions, Meteor.startup, and Meteor.publish (eg, your code may be run multiple times simultaneously on multiple servers, etc). Client-side should be more predictable though.

@scottburch

This comment has been minimized.

scottburch commented Oct 1, 2012

Thank you. I put a monkey patch in my code to do just that so that I can just remove it when you have merged it into the auth-devel branch.

Thanks again for your work.

@scottburch

This comment has been minimized.

scottburch commented Oct 1, 2012

A bit sloppy, but if anyone needs it until the merge, this is my monkey patch. I stole the logic for isNotStorable() from the session package code. The .wrap() method is a standard functional programming wrap around a function.

(function () {
    // Monkey patch session to store objects
    function isNotStorable(value) {
        return (typeof value !== 'string' &&
            typeof value !== 'number' &&
            typeof value !== 'boolean' &&
            value !== null && value !== undefined);
    }

    Session.set = Session.set.wrap(function (cb, args) {
        var key = args[0];
        var value = args[1];
        cb(key, isNotStorable(value) ? JSON.stringify(value) : value);
    });

    Session.get = Session.get.wrap(function (cb, args) {
        var value = cb(args[0]);
        try {
            return JSON.parse(value);
        } catch (e) {
            return value;
        }
    });
}());
@glasser

This comment has been minimized.

Member

glasser commented Oct 6, 2012

I merged in Greenspan's ContextSet changes so it should be reasonable to fix this now.

@glasser glasser closed this Oct 6, 2012

glasser added a commit that referenced this issue Oct 6, 2012

glasser added a commit that referenced this issue Oct 9, 2012

Merge branch 'devel' into auth
Pull in the issue #215 fix (Sessions can contain non-scalar objects)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment