Handle bookmarks if app code has changed

[slopp]

RSC now supports bookmarkable state and will be shipping with this support next week. In RSC it is really easy for user's to update code or rollback code. This means today it'd be easy for a user to deploy a new version of an app that breaks existing bookmarks.

There are a few ways I can think of that we could address this user story:

1. We do nothing. Bookmarkable state is something that users have to opt-in to by adding code. That means it is on them to be smart about updating apps that have bookmarks in them.

2. We could make changes in RSC to warn the app author during a rollback or during a re-deploy if that app has bookmarks. This would require a lot of invasive changes in RSC and potentially the IDE.

3. We could make some changes to Shiny to try and handle an update better. For example, if there are discrepancies between the bookmark and the app, we could load the default with a warning. The changes would be picked up by SSP, RSC, and OS SS.

My vote is for #3.

@trestletech @wch could you weigh in on this issue?

[jcheng5]

In regards to 3, it's not clear to me how detecting discrepancies would work. There's no manifest or anything that declares what values are expected for a particular version of an app; think of it instead as a series of callbacks at bookmark time, and a series of callbacks at restore time (and the restore callbacks can take place over an arbitrarily long period of time).

I'm not saying it's not possible to do something helpful, @wch might have some ideas, but it's definitely not obvious to me.

What about 1.5: nothing about the code changes, but we write a detailed article with recommendations about how you can handle this situation? (I imagine this being, you save an explicit version number into the bookmark data, rev the version in your code if you make warning- or error-worthy changes in your app, and then warn/error when restoring if the versions differ)

[slopp]

@tareefk mentioned that we might already surface the application version, @wch do you know if thats the case?

I'd be happy to do option 1.5 for now. I think the best place would be to add to the existing bookmarkable state articles (something like "Handling bookmarks and application updates"). I guess its a relatively safe bet that shiny devs implementing bookmarkable state read these articles. Though I'm always surprised in calls about how little our customers read the doc.

You can assign the issue to me and I'll make a point of touching base with @wch before I do anything.

[bborgesr]

My two cents: I'd vote for 1.5 too. Both options 2 and 3 require non-trivial code changes to one or more of our repos, and even if we went all the nine yards, there would probably be a billion edge cases...

One thing that occurs to me is that RSC could add an extra bookmarked argument to bookmarkable apps (which should be a simple check, right?) with the app's version (at the very end of the URL). So the bookmarked URL would contain this info and direct you to that app's version. The user can manually enter the latest app version in the bookmarked URL if he wants, but then he knows there may be breakage... Also, this way, we can ensure bookmarks always work... But I have no idea how feasible this is for the Connect team.

[jcheng5]

We've discussed the "add RSC bundle version to bookmark URL" idea before. The problem is knowing what the correct behavior is for a particular deploy. If bookmarks always take you to the version of an app that did the bookmarking, that opens the door to apps running after it is no longer safe to do so (maybe they were changed due to security issues or because of a critical data-destroying bug or the database schema changed so the app would no loner work at all or...).

On the other hand, if bookmarks always take you to the newest app version but you get a warning if that version is different than the version that the bookmark was created with, then that would be annoying if you make a simple fix to the app like cleaning up some CSS or fixing a typo in a label.

I don't think there's a magical substitute for app authors controlling this process explicitly, but it is certainly up to us to educate them.

[wch]

I can see two broad ways that bookmarking could be used:

1. As a historical record. In this scenario, the author wants to be able to save and restore a specific version of an app with a specific state. This is analogous to a git commit hash: it refers to an immutable state, from a particular time in the past.
2. As a shortcut to get a continuously-developed and continuously-used application in a particular state. I could imagine this being used for, say, a dashboard.

I think these are both valid and valuable use cases. I agree with Joe that there are some potential security issues if old versions of apps are allowed, but I that would apply to a minority of Shiny apps out there. On the other hand, if with bookmarking you could restore an old version of an app, that makes it simpler for app authors because they don't have to worry about making breaking schema changes to their app.

Here's one possibility: we make (1) available only with saved-to-server bookmarking, and the app author needs to opt into it. We'll try to make it clear to them the trade-offs for doing so. The default behavior would be (2).

Now some technical stuff:

If RSC exposed the application's bundle ID, it would be possible for the bookmarking code to record that in the saved state, and then on restore, give a warning if the current app version differs from version at bookmark time.

With regard to 3, I don't see a clear way to do it. It is possible to track which inputs have been restored from a bookmarked state, so you might be thinking, "at the end of the restore phase, give a warning if any of the available input values weren't used." The problem is that there's no pre-determined time at which we can say that the restore process has been completed. For example, imagine an app with two tabs and in each tab theres dynamic UI, and that UI contains an input. Then the input that's on the second tab won't be restored until the time that someone navigates to it, and that could happen whenever. Here's an example to illustrate:

library(shiny)
ui <- function(req) {
  fluidPage(
    bookmarkButton(),
    tabsetPanel(
      tabPanel("tab1", uiOutput("ui1")),
      tabPanel("tab2", uiOutput("ui2"))
    )
  )
}
server <- function(input, output) {
  output$ui1 <- renderUI({
    print("rendering 1")
    sliderInput("s1", "Slider 1", 1, 100, 50)
  })
  output$ui2 <- renderUI({
    print("rendering 2")
    sliderInput("s2", "Slider 2", 1, 100, 50)
  })
}
shinyApp(ui, server, enableBookmarking = "url")

One more limitation: if people add onBookmark and onRestore callbacks, the state object that's passed to those functions is simply an environment to which values are written and read. Unlike the inputs that are automatically restored, the state object doesn't have the needed machinery to track out what values have been read.

[slopp]

@wch I think those cover the two use cases. There is a bit more overlap between the two use cases than the way you presented them.

To flush this out, here's a user story:

Imagine you're a BI analyst that is using a shiny app built by the data science team. You get some awesome insights so you bookmark the state. The next week you either go to a meeting planning to use the link or you send an email to your boss, Mr. VP. Unfortunately for you, the app author has updated the app. A few things could happen (in order of badness):

1) [hypothetical today] Your bookmark could bring up the old version of the app. All is good as long as the app's state didn't change due to live data or something.

2) [hypothetical today] You get a warning that the app has been updated since you bookmarked your code. You fire off a nasty-ish email to the irresponsible data scientist who pushed an update with no warning

3) [possible today] You bookmark causes the app to error. Embarrassing. You fire off an even nastier email to the data scientist and in turn the data scientist potentially complains to RStudio

4) [most likely today] Your bookmark works, but not in the way you'd expect. Suddenly you're half way through your presentation and you get confused because the results don't match what you expected

In all of these scenarios there are a few things to keep in mind: A bookmark doesn't equal reproducibility for most apps, since most apps depend on outside data. In all the scenarios the data scientist shares some of the blame for having included a bookmark and not thinking about that fact when he makes his changes. Solution 1.5 tries to save the data scientist by calling attention to this concern and providing a best practice.

[bborgesr]

@slopp, we talked about this as a team some more yesterday and we're proud to introduce option 1.75. In addition to a very clear set of best practices documented in articles (option 1.5), we also make some minor-ish code changes to Shiny to allow the following (from here on, this is just a mockup and highly likely to differ from the actual implementation):

• change the enableBookmarking() function to take in an extra argument, called appVersion (defaults to something like 0.10). In order to make this less confusing with RSC app versioning, we could also call it bookmarkVersion and have it start at 0 (or another sensible default that doesn't look like an actual version). The first time you deploy your app to Connect, nothing particularly interesting happens. Under the hood, though, you can think of this new param as an additional input to the app, on which conditional behavior can be anchored. For the first time you deploy though, it will look exactly as it does now.

• but now let's say that you made some changes to the app and you want to redeploy it. Here, there's two possibilities:

  • you haven't changed the bookmarkVersion param. This will cause RSC to ask you (only RSC change required) if you're sure that you don't need to change it (this is a valid case - basically you shouldn't change the bookmarkVersion param if any previously bookmarked URLs will still produce the same results with the changed app -- for example, if you only changed some css). If this is indeed the case, you proceed and all is good with the world.

  • you have changed the bookmarkVersion param (either by yourself or because RSC prompted you to do it). This always has to be paired with another action, which is to specify how an older URL should behave in this new app. I think the best default is to completely error out (but that's a discussion for all of us to have later). This would probably be a third param to enableBookmarking(). Here's a quick mockup of what this may look like:

  # default
  enableBookmarking(store = "server", bookmarkVersion = 2, function(version, keypairs) {
    if (version != 2) 
      stop("This app has been updated and old bookmarked URLs no longer work.")
  })
  
  # custom (and more useful!)
  enableBookmarking(store = "server", bookmarkVersion = 2, function(version, keypairs) {
    if (version == 0) {
      stop("This app has been updated and old bookmarked URLs no longer work.")
    } else if (version == 1) {
      if (keypairs[["oldInput"]] == "unsupportedValue") {
        showNotification(
          paste("'oldInput' can no longer have the value of 'unsupportedValue'.",
                "Please select a different value.")
        )
      }
      # whatever is not explicitly enumerated here will fail silently, as happens now
    }
  })

How does this sound?

We think it's possible to do this within the next couple of months (Winston will get on it when he can).

@jcheng5, @wch, please correct me if I explained this wrong

[slopp]

@bborgesr can I propose version 1.65? 👍

I really like this mock-up. I think we could introduce the changes in Shiny and then see if it'd be worthwhile to introduce the checking / warning in the deployment process. Right now that check would have to take place inside of the rsconnect package, and any UI would need to be wired into the IDE. https://github.com/rstudio/rsconnect/blob/master/R/deployApp.R#L359-L372 Connect just accepts applications and runs them with runApp. We'd also need to check the impact on shinyapps.io.

One additional comment. I talked to Nathan, I think if we do add bookmarkVersion, we should add an option to display the version and last updated date (and maybe even maintainer) in a footer of the app. This would:

a) Promote the best practice of versioning apps and increase visibility of versions

b) Add a nice "production-worthy" feature for Shiny apps (something we could point to on a call and say, "yea, Shiny isn't just a toy, look at how this app is deployed, versioned, and maintained"

c) Encourage the idea that the reliability of data / apps / insights decreasing through time, which was one major take away from the Gartner conference about how Shiny and RSC can support governance best practices.

enableBookmarking(store = "server", bookmarkVersion  = 1, showVersion = TRUE, function(...))

[bborgesr]

@slopp I'm fine with option 1.65. Although we could also go for option 1.70, in which every redeployment of an app will have a non-so-prominent, but still present, message saying something like:

If you are redeploying a bookmarkable app, make sure you have specified the correct bookmarkVersion to ensure backward compatibility (see [link to article]).

As for showVersion = TRUE, it sounds like a good idea. But it's more reminiscent to me of a display.mode than something else that is entirely new (it's kind of a barebones showcase mode). Decoupling this from bookmarking also has the advantage that you could have this nice feature even if you don't use bookmarking. Which brings me to the more worrisome thought in here: which version number are we talking about: the one provided by RSC when you click "Source Versions," or the one provided by the app author in the bookmarkVersion param? If we want all of this to involve only changes to Shiny (and not RSC et al), we cannot have the former. However, I can see app authors (and users) getting confused with this... (Maybe that's the price to pay?)

Also, if we want to give finer control, we could do something like allowing display.mode to be a function call, instead of just a string:

# 1. normal
shinyApp("MyApp", options(list(display.mode = "normal")))

# 2. showcase
shinyApp("MyApp", options(list(display.mode = "showcase")))

# 3. "custom" footer
shinyApp("MyApp", options(list(display.mode = 
  footer(maintainer = "Sean Lopp", bookmarkVersion = NULL, lastUpdated = Sys.time()))))

What do you think?

[slopp]

Decoupling this from bookmarking also has the advantage that you could have this nice feature even if you don't use bookmarking.

Agreed.

Which brings me to the more worrisome thought in here: which version number are we talking about: the one provided by RSC when you click "Source Versions," or the one provided by the app author in the bookmarkVersion param?

I think we should consider this "version" something separate from what is displayed in RSC. RSC shows the bundle ID, which is just a random, unique ID assigned anytime a user deploys content.

The version we're discussing here is a unique tag that the app owner controls and only increments when necessary.

If the UI label in Connect, "Source Versions", becomes confusing, we could change the UI to "Source Bundles" or something. But I think the concepts are distinct enough, especially if we document the scenario where bundle # would increment but version would not.

I don't think anyone today (in practice) publishes with display mode, but I think it could be a reasonable fit. We'd just have to encourage the use and ensure that the footer doesn't dramatically visually change the app.

If we pull the version out of enableBookmarking, we'd need a way to link the two things together. You wouldn't want users to have to specify the app version in two places. Perhaps:

setVersion(1.0)
...
enableBookmarking(store = "server", function(getVersion()){...})
...
shinyApp(ui, server, options(list(display.mode = 
  footer(author = "me", version = getVersion(), updated = Sys.date()))))

[bborgesr]

Re:

But I think the concepts are distinct enough, especially if we document the scenario where bundle # would increment but version would not.

Cool, the most important thing would really be to make sure that the two concepts are not getting mixed up in users' head. You get a lot more contact with users than I do, so if you think that won't be an issue, I trust your judgment.

Re:

If we pull the version out of enableBookmarking, we'd need a way to link the two things together. You wouldn't want users to have to specify the app version in two places.

I agree, but I wouldn't introduce a new function for it, we'd just document that you should declare it once, at the top of the app (see example below). Also, this may be just a typo, but in your example, you supply getVersion() as as argument to the anonymous function in enableBookmarking(). This is a mistake (although, we're both just talking about mockups). That callback would be called when a bookmarked URL of that app is used to launch it, so the argument version takes the value of whatever is in the bookmarked URL (you don't know it ahead of time). The useful thing would be to compare that argument to the current version number and then indicate what to do for each possible scenario. For example:

vrs <- "1.0"
...
enableBookmarking(store = "server", function(version){
  if (version != vrs) ...
})
shinyApp(ui, server, options(list(display.mode = 
  footer(author = "me", version = vrs, updated = Sys.date()))))