R-initiated page changes (and other things...)

[agwells]

Hi!

I've recently been doing some client work which has led me to implement some new features for shiny.router, which I'd be happy to contribute back to the project. The code on this branch is not 100% ready to be merged, but @filipstachura noticed my Github fork and suggested we start the upstreaming discussion now, since it is rather a big chunk of changes. :)

The specific features I've added are:

1. Let R code change the current page
2. Let R code read query params from the page URL
3. Let the page start showing any of the routed URLs, rather than forcing a start on the "default" one.
4. Convenience functions to check which page is currently loaded (in order to avoid performing intermediary calculations for pages that won't be displayed.)
5. Removing the shinyjs dependency, because its license has changed to AGPL (and while I personally love the AGPL, I've been working on this for a client who does not want to AGPL-license their site).
6. I changed the page.js settings so that it uses hashbang URLs (aka "escaped fragment"), and disabled the fullpage click-handler, because I found that wouldn't work correctly with the Shiny DataTables DT package.

I also monkeyed with the JS binding so that it's not based on the content of a hidden <input type="text">. (It currently does create a <input type="hidden">, but that's just a placeholder because Shiny's JS side expects there to be one; it doesn't read or write to it.) But now that I've learned more about Shiny, I think maybe I don't gain much by this approach, and it may be more foolproof to go back to the original input-tag based approach.

Please take a look, and let me know any problems or suggestions for change. :)

Cheers, Aaron

[agwells]

Update: I took a closer look at shiny::getUrlHash() and shiny::updateQueryString() and realized these can be used together to reactively respond to changes in the URL's hash, and to update the hash from R. Which means we can remove all the custom JS and the input tag! (Though note this only works with "hashbang" URLs)

[marekrogala]

@agwells Awesome, thanks for the PR! We'll take a look and share our thoughts next week

[marekrogala]

@agwells thanks for the PR; we'll have to plan the roadmap for shiny.router after New Year and then we'll get back to this PR

[dokato]

Hi @agwells . I'm reviewing your PR and noticed that now when running examples after pressing any page I got "Not Found" message. After the code being refactored so much it's hard to find a bug for me. Could you please take a look? Does the currect implementation require changes in server initialization?

[dokato]

From logs it looks that regardless what I press I got:

New raw hash:  
Empty hash.
Path:  /
Path recognized!
shiny.router main output. path:  /

[dokato]

I noticed that actually no logs are generated when changing hash, so looks like this code in not evaluated:

    observeEvent(
      ignoreNULL = FALSE,
      ignoreInit = FALSE,
      # Shiny uses the "onhashchange" browser method (via JQuery) to detect
      # changes to the hash
      eventExpr = shiny::getUrlHash(session),
       ...
      )

which means that it doesn't detect change of session.

[dokato]

Gotcha, now you need to explicitly add the hash to the link:

menu <- (
  tags$ul(
    tags$li(a(class = "item", href = "/", "Page")),
    tags$li(a(class = "item", href = "/#!other", "Other page"))
  )
)

Maybe we should come back to previous solution with /#!/link?

[agwells]

Hiya @dokato,

Thanks for taking a look! I should clarify, this code is not merge-ready right now. Since I'm making such a big change, I opened the PR early, in order to get some feedback on whether you'd want to merge it in eventually, and if so, what changes I'd need to make. :)

You're right, one of the non-back-compatible changes is that, with the removal of page.js, it no longer automatically modifies all the links on the page to capture them for shiny.router. I had forgotten about that! :)

I did that because I was finding that the page.js code conflicted with the DT data-tables package. I didn't dig into that too deeply, but it looked like the problem was that page.js works by putting an onclick handler over the entire page, but DT also puts an onclick handler over its table and that stops the page.js one from working for links on text in the table. So, I switched to #! links because those can work without an onclick handler. I meant to, eventually, add a convenience function to make "#!" links automatically, but I haven't done that yet. I also haven't updated any of the examples to work with the new system.

It probably would be a good idea to add an automatic link-modifying option back in, whether that's via page.js or other code.

[agwells]

Hm... looking back on it now, though, I wonder if a better solution to the DT onclick conflict, would be to add a function to shiny.router that can place a page.js onclick handler directly onto a specific link? Then I could use that on the links in my datatable, and they'd work without having to resort to hashpaths.

[dokato]

@agwells Thanks for reply! Actually I like the new approach with shiny hash functions. I see some room for improvement on this PR, but in general the code looks pretty good to me. And good idea with those log messages, btw.

I wonder if a better solution to the DT onclick conflict, would be to add a function to shiny.router that can place a page.js onclick handler directly onto a specific link?

But that would require calling that function by user only on links in the DT? I think it would be better to be consistent. The current code deals with that problem, right? Let me think again if the approach without page.js is robust enough, and I will come back to that in a few days with decision in which direction should we pursue.

[agwells]

Hi again @dokato,

I wonder if a better solution to the DT onclick conflict, would be to add a function to shiny.router that can place a page.js onclick handler directly onto a specific link?

But that would require calling that function by user only on links in the DT? I think it would be better to be consistent. The current code deals with that problem, right?

The current code, with hashpaths, is consistent in that it requires you to hash-ify every link manually. :)

My thinking on switching back to page.js was that, if the page.js global onclick works for most pages, then it would be best to keep that, and add the manual onclick inserter for the few cases where the automatic global onclick doesn't work.

That said, I've remembered the other big reason I switched to hashbangs, was feature 3 on my list from November: being able to start the app on a subpage, i.e. bookmarking and going directly to "http://report.example.com/item?id=31" instead of "http://report.example.com/". I wasn't able to get that working in Shiny; any attempt to go directly to a page other than "/" (or whatever I entered for the uiPattern option) gave a 404 error.

Although, in doing some testing now and reading the Shiny source code... it looks like it is possible to make a Shiny report handle multiple paths. But it requires specifying a partial regex to the uiPattern param of shinyApp(), and modifying your ui into a function that takes a req argument and returns NULL if req$PATH_INFO isn't one of the routed paths. So... a bit of work, but possible.

The main reason to avoid hashpaths is that they use the fragment portion of the URL differently from how the URL standards say it should be used; also Google (which originally popularized them) has now deprecated them.

Cheers, Aaron

[dokato]

@agwells so after thinking it through I believe that we should stay with shiny hash implementation. Google won't support fragments in search, but they will be still valid in Chrome as I understand. We shouldn't worry about that now. Regarding the hash-ifying links manually I was thinking about some extra function e.g. route_link with which user would have to use to encompass the link leading to routing page. Or maybe some django-like solution: https://docs.djangoproject.com/en/2.0/ref/urlresolvers/. What do you think?

[dokato]

To not let grow this PR too big I decided to merge it to dev branch, as we want to move router in that direction anyway. @agwells if you have some more amendments to add, please PR it to dev branch directly now.