shinyworks / cookies

Use Browser Cookies with 'shiny'
https://shinyworks.github.io/cookies/
Other
32 stars 4 forks source link

Proposal for using `session$userData` instead of finding the root `session` #62

Open gadenbuie opened 1 year ago

gadenbuie commented 1 year ago

I caught your talk at ShinyConf2023 (which was great, btw!) and I've been thinking a bit about your callout of needing to find the root session object to get cookies to work inside modules.

I have an MVP proposal for an alternate solution. Because cookies are global settings specific to the user session, I think session$userData is a very appropriate place to store and access the cookie values. The $userData is described in the docs as

userData An environment for app authors and module/package authors to store whatever session-specific data they want.

It's also helpfully included in the session object available in modules, so session$userData can be used everywhere.

My proposal, in a nutshell, is this:

  1. Upgrade the input$cookies list to a reactiveValues()
  2. And make it available session$userData$cookies.

Technically only the second option is required, but under the current design I believe any observation of an individual cookie will introduce a reactive dependency on all of the cookies. Here's a really small example that shows that updating either of the inputs whose values are stored in the cookies causes any observers accessing a single cookie to update.

library(shiny)
library(bslib)
library(cookies)

ui <- page_fluid(
  h1("Cookie Demo"),
  textInput("first_name", "First name"),
  textInput("last_name", "Last name")
)

ui <- add_cookie_handlers(ui)

server <- function(input, output, session) {
  ts <- function() strftime(Sys.time(), "[%F %T] ")

  observe({
    message(ts(), "first_name: ", get_cookie("first_name"))
  })

  observe({
    message(ts(), "last_name: ", get_cookie("last_name"))
  })

  observe(set_cookie("first_name", input[["first_name"]]))
  observe(set_cookie("last_name", input[["last_name"]]))
}

shinyApp(ui, server)

Imagine individual cookie values are stored in a reactiveValues() list in session$userData. In that case, users could call get_cookie() or could directly access a single reactive cookie value without taking a complete dependency on input$cookies.

server <- function(input, output, session) {
  # call a function that initializes `session$userData$cookies`
  add_cookie_handlers_server()

  observe({
    message(ts(), "first_name: ", session$userData$cookies$first_name)
  })

  observe({
    message(ts(), "last_name: ", session$userData$cookies$last_name)
  })

  # more logic...
}

In the above example, observing first_name won't introduce a dependency on last_name. And you don't have to go looking for the ancestor session object, session$userData$cookies is available everywhere.

Here's a proof-of-concept for add_cookie_handlers_server():

add_cookie_handlers_server <- function(
  session = shiny::getDefaultReactiveDomain()
) {
  input <- session$input
  session$userData$cookies <- reactiveValues()

  observeEvent(input$cookies, {
    for (item in names(input$cookies)) {        
      session$userData$cookies[[item]] <- input$cookies[[item]]
    }
  })
}

You could also imagine adding an observer of the reactive values list that would handle calling set_cookie(), so that the user (or you) could do something like

session$userData$cookies$first_name <- "Garrick"

to update the cookie through an observer added in add_cookies_handlers_server(). (That would probably require similar hacks around comparing the current and new versions of the reactive values list.)

Here's a demo app that demonstrates how you'd use this in practice (although you might want to hide session$userData$cookies inside get_cookie() instead of calling it directly like I am here).

library(shiny)
library(bslib)
library(cookies)

nameUI <- function(id) {
  ns <- NS(id)
  tagList(
    uiOutput(ns("input_name"))
  )
}

nameServer <- function(id) {
  moduleServer(
    id,
    function(input, output, session) {
      ns <- session$ns

      output$input_name <- renderUI({
        message("Updating ", id, " name input module")

        cookie_name <- ns("name")
        name <- session$userData$cookies[[cookie_name]]
        label <- sprintf(
          if (isTruthy(name)) "Change your %s name" else "What's your %s name?",
          id
        )

        textInput(ns("name"), label, value = name)
      })

      observeEvent(input$name, {
        cookies::set_cookie(ns("name"), input$name)
      })

      return(reactive(input$name))
    }
  )
}

ui <- page_fluid(
  h1("Cookie Demo"),
  uiOutput("intro"),
  nameUI("first"),
  nameUI("last")
)

add_cookie_handlers_server <- function(session = shiny::getDefaultReactiveDomain()) {
  input <- session$input
  session$userData$cookies <- reactiveValues()

  observeEvent(input$cookies, {
    for (item in names(input$cookies)) {        
      session$userData$cookies[[item]] <- input$cookies[[item]]
    }
  })
}

server <- function(input, output, session) {
  add_cookie_handlers_server()

  first_name <- nameServer("first")
  last_name <- nameServer("last")

  output$intro <- renderUI({
    req(isTruthy(first_name()) | isTruthy(last_name()))
    intro <- "Hello"
    if (isTruthy(first_name())) {
      intro <- paste0(intro, ", ", first_name())
      if (isTruthy(last_name())) {
        intro <- paste(intro, last_name())
      }
    }
    p(paste0(intro, "!"))
  })
}

ui <- cookies::add_cookie_handlers(ui)
shinyApp(ui, server)
jonthegeek commented 1 year ago

This is great! I've been on the verge of this answer (see my question during a talk on Wednesday asking if anybody has example use cases for session$userData 🤣)

I'll need to make sure I take HttpOnly cookies into account for the implementation (they come in with the request but aren't available in the JS), but I'm PRETTY sure everything you suggest will still apply.

Thank you so much!

jonthegeek commented 1 year ago

Hahahaha I thought I'd thought about this before, and found what steered me away: https://twitter.com/JonTheGeek/status/1579287872762699780 🤣

gadenbuie commented 1 year ago

You're welcome! You mean this comment?

hadleywickham says "You might see advice to use session$userData (to work around NS)... Be wary of such advice..." but I think I have a legit reason to muck around in session. He's talking about modules, but I think maybe cookie handling makes sense there? #RStats #RShiny

I'd argue you have a very valid case here. The cookies are global to the session and are limited to a specific user's session. (They'll even change if the user opens the app in a different browser.) Really, I don't think there's much difference between using session$userData and finding your way back to the root session 😆

I'm going to edit my answer above, but Winston pointed out to me that when reactive values are set there's already an internal identical() comparison, so the input to reactive values conversion can be simplified a little bit.

jonthegeek commented 1 year ago

Yeah, I think I scared myself away from it, forgot about it for a while, and then ended up using the .root_session() hack when I came back to make it work 🙃

jonthegeek commented 1 year ago

Implementation notes (which, it turns out, go on a journey! at the end I think I decided not to implement this, per se):

Maybe .root_session() will remain the right answer, I just need/want to split things up to avoid over-reactivity?

jonthegeek commented 1 year ago

... But OK, probably having it all in a single cookies input, and then observing that and parsing it out, will work. I should be able to auto create the observer when they first intetact with cookies still, to avoid having to add a function to the server. I think. I'll try it out probably tomorrow!

mbjohnsonmn commented 1 year ago

Thanks for this package! I am using it to implement cookies in fairly complex app and am struggling with 'over-reactivity' also. I am able to follow some of the conversation above, but not all. Has there been any new developments? Thanks!

jonthegeek commented 1 year ago

Thanks for this package! I am using it to implement cookies in fairly complex app and am struggling with 'over-reactivity' also. I am able to follow some of the conversation above, but not all. Has there been any new developments? Thanks!

Great to hear!

I started implementing this, but ran into some hairiness, and then got pulled away to other projects. I've had a couple comments over this way + I'm about to update the shiny app that led me to create this, so I'll be back very soon to make sure I'm not doing anything overly complicated 😊

I'm HOPEFUL that I can clean up quite a lot of code my next time through. I've learned SOOOOO much since the last time I worked on this package...

jonthegeek commented 1 year ago

@mbjohnsonmn See #65 (and #64 where I logged that I need to come back to this). It SHOULD be better behaved now, although it definitely isn't quite how I want it just yet. This fix MIGHT help deal with some of your issues, though!