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

Add session$userData feature #1513

Merged
merged 3 commits into from Dec 15, 2016
Merged

Add session$userData feature #1513

merged 3 commits into from Dec 15, 2016

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Dec 15, 2016

Fixes #1512

This implements session$userData as an environment, not reactiveValues. @wch @bborgesr feel free to weigh in. I could go either way.

@bborgesr
Copy link
Contributor

I vote for leaving it as an environment. Joe, in #1512, you said:

Only question I have is whether session$userData should be a regular environment, or a reactiveValues? (If the former, if you needed reactivity you could create a reactiveValues object and put it into a slot in the regular environment; but if the common case is needing reactivity it might be nicer to do the latter.)

Your parenthetical is good enough for me. I think if people need it, they should go through the trouble of doing what you suggest. The reason I don't think this should be the default is because I think that, in most cases, this will be used just for counters and regular objects, in which case, it would be a pain to have to do isolate(session$userData$counter) each time you want to access the value without taking a dependency on it (or even when you're outside of a reactive context -- if you're just accessing the value from a regular function, for example).

@wch
Copy link
Collaborator

wch commented Dec 15, 2016

I think using an environment is a good idea.

@wch wch merged commit accd70d into master Dec 15, 2016
@wch wch deleted the joe/feature/session-userdata branch December 15, 2016 18:50
@wch wch removed the review label Dec 15, 2016
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.

None yet

3 participants