Handle search/hash URL components and custom app locations #1

gadenbuie · 2020-07-15T21:22:38Z

I discovered this package about a month ago and have really been enjoying it. You did some awesome work getting it set up and proving the feasibility of working around shiny's URL routing. Very cool stuff!

I've added a few features that I thought I'd share with you if you want to merge back in. I'd be happy to do any follow up if you have code review feedback.

Hopefully my commit history tells a reasonable story, but in a nutshell:

I fixed installation from GitHub via remotes as the inst/tmp files were breaking install
I updated the JS to transparently modify just the URL path without touching the query or hash strings. This means you can go to <app>/<page>?id=99 and the ?id=99 will be preserved through the various redirects.
Shiny apps often live at paths other than the root directory, so I added app_path to paths() that lets the user declare the path where their app is running. This makes it possible to deploy on shinyapps, e.g. gadenbuie.shinyapps.io/blaze_app/<path> but does require the user declare app_path = "blaze_app" at runtime.
Finally, I gave pushPath() a mode argument, like shiny::updateQueryString() except with the default being "push". This makes it easier to redirect from / to an initial page, e.g. /first, without having the first / in the history stack. Oh and I also updated the JavaScript to return the URL pathname if the history state isn't written by the previous pusher.

I'm not sure where these were used but they caused issues when trying to install via remotes::install_github()

when redirecting into the shiny app or when triggering a page change from within the app

Bumps roxygen2 to 7.1.0

…pdates

Shiny apps are often publicly hosted in sub-directories. Specifying app_path ensures that blaze() parses the browser's URL path in relation to the path where the app is hosted.

getPath() always includes the leading slash, so observePath() adds the slash if missing. This also fixes an issue with routes when app_path is set

And also store pathname and uri in history state so that we can easily fall back to window.location.pathname if event.state is null. This catches the case when shiny::updateQueryString() is used to update the query string because the JS for that function sets the history stack item state to `null`.

nteetor · 2020-11-17T21:03:31Z

R/paths.R

  UseMethod("as_paths", x)
 }

+#' @export


I would like to keep as_path and its implementations internal.

This exports the S3 methods but not the generic, following the advice in R Packages.

I see the second bullet, thank you for the link. I'm not wholly convinced and the wording is rather vague. Do you know anymore about why this is good practice?

As of R 4.0 it's actually required. The #' @export tag on S3 methods doesn't export the method, it registers the method with S3method() in NAMESPACE and this step is now required, even for internal generics, for the methods to be found.

This thread has a bit more discussion: r-lib/devtools#2293

Thank you for the discussion link. I had not known the change would affect internal generics in this way.

nteetor · 2020-11-17T21:05:45Z

inst/www/js/blaze.js


  window.addEventListener("popstate", function(event) {
-    sendState(event.state || "/");
+    sendState(event.state.replace(/[?#].+$/, '') || "/");


What was the motivation for dropping the search and hash components?

I think maybe this was resolved but IIRC the goal here is to ignore the URL hash component

R/blaze.R

nteetor · 2020-11-17T21:18:07Z

inst/www/js/blaze.js

+  var pathURI = function(redirect, {params, hash} = getURLComponents()) {
+    if (!redirect || redirect == window.location.pathname) {
+      return false;
+    }
+    if (params.toString()) redirect = redirect + '?' + params;


Because params and hash are never specified in the calls to pathURI() I would prefer the arguments are removed and the variables assigned in the body of the function.

Suggested change

var pathURI = function(redirect, {params, hash} = getURLComponents()) {

if (!redirect || redirect == window.location.pathname) {

return false;

}

if (params.toString()) redirect = redirect + '?' + params;

var pathURI = function(redirect) {

if (!redirect || redirect == window.location.pathname) {

return false

}

let {params, hash} = getURLComponents();

if (params.toString()) {

redirect += `?${ params }`;

}

That makes sense. I think I had used at one point and then refactored but kept it around in case we'd want to provide params or hash. Which clearly isn't necessary anymore.

nteetor · 2020-11-17T21:21:00Z

R/zzz.R

@@ -1,3 +1,3 @@
-.onLoad <- function(pkg, lib) {
+.onLoad <- function(lib, pkg) {


Another one for a cleanup pull request.

nteetor · 2020-11-17T21:22:20Z

R/url.R

 }

-#' @rdname observePath
-#' @export


I realize the function does nothing at the moment. This is a change I especially want to exclude from a feature pull request.

nteetor · 2020-11-17T21:26:09Z

inst/www/js/blaze.js

        var uri = target.getAttribute("href");

        if (uri !== window.location.pathname) {
-          sendState(uri);


What is the motivation for no longer sending the path in this situation when a link is clicked?

I'm pretty sure this was just a bit of clean up so that sendState() is always called by _path()

nteetor · 2020-11-17T21:37:11Z

R/paths.R

  on.exit(setwd(old))

-  if (!is.null(app_path)) {
+  .globals$app_path <- if (!is.null(app_path)) {


I'd prefer not to use this sort of short-hand.

nteetor · 2020-11-17T21:48:46Z

R/url.R

 #'
 #' @export
-pushPath <- function(path, session = getDefaultReactiveDomain()) {
+pushPath <- function(path, mode = c("push", "replace"), session = getDefaultReactiveDomain()) {


I would prefer to not support replace. The replace behaviour feels unexpected or non-standard. What's the motivation for including a replace option?

This was primarily for parity with mode in shiny::updateQueryString(). Secondarily, it's conceivable that an app dev might want to use replace to update the URL during some initialization process. That was my use case, it was possible to land on one page with a set of inputs that would redirect to another page and I didn't want the first state to be on the history stack.

Good point, thank you for outlining a use case. I might introduce a second function then replacePath().

nteetor · 2020-11-17T22:48:35Z

There's a lot of content here, so thank you for the time and energy you put in. I think it's worth breaking this pull request into separate PRs. I know there are some additions I have no reservations adding and other pieces I'd like to talk further about.

gadenbuie · 2020-11-23T18:45:02Z

Hi Nate, good to hear from you! I saw that you've already started incorporating some of the ideas from here into blaze. That sounds great to me, feel free to use whatever you want from this PR. I left comments on the questions above and I'll be happy to answer anything else that comes up.

gadenbuie added 20 commits June 1, 2020 15:54

buildignore inst/tmp/

63895b0

Remove inst/tmp files

72a9b39

I'm not sure where these were used but they caused issues when trying to install via remotes::install_github()

Maintain search params and hash components of URL

16f302b

when redirecting into the shiny app or when triggering a page change from within the app

Document paths(), export paths(), blaze() and as_path() S3 methods

b842737

Bumps roxygen2 to 7.1.0

Bump dev version, update authors

248d965

Remove search and hash components of URL when sending browser state u…

c525610

…pdates

Update documentation to use roxygen function links

733d41a

Fix function signature of .onLoad() to appease R CMD check

e9ee710

Adds {utils} to dependencies for packageVersion()

01aa30b

Unexport param(), it's not ready yet

13a3a14

Add at least one basic test

c355ff7

Don't update path if same as current, also send updates back to server

56ba9dc

Add app_path argument to paths()

d2b70aa

Shiny apps are often publicly hosted in sub-directories. Specifying app_path ensures that blaze() parses the browser's URL path in relation to the path where the app is hosted.

Redirect pages need to redirect to /app_path

1019a39

Ensure that getPath() returns "/" when app_path is set

e8b168c

Prepend app_path after checking path input in pushPath

8d0c7cd

Add path_app() and ensure pathLink() respects global app_path

249eaae

Reset app_path if app_path = NULL when calling paths() in same session

7938bf2

observePath() correctly handles leading route path slash

99a4265

getPath() always includes the leading slash, so observePath() adds the slash if missing. This also fixes an issue with routes when app_path is set

nteetor reviewed Nov 17, 2020

View reviewed changes

		@@ -1,3 +1,3 @@
		.onLoad <- function(pkg, lib) {
		.onLoad <- function(lib, pkg) {

Handle search/hash URL components and custom app locations #1

Are you sure you want to change the base?

Handle search/hash URL components and custom app locations #1

Uh oh!

Conversation

gadenbuie commented Jul 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nteetor commented Nov 17, 2020

Uh oh!

gadenbuie commented Nov 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants