[WIP] Inspect environment variables and set routing prefixes, add catchall endpoint #53

rpkyle · 2019-02-21T15:12:54Z

This PR will introduce a 'catchall' endpoint /*, which will be added as the final handler in the routr stack.

Additionally, DashR does not inspect local environment variables when initializing an app; routes_pathname_prefix and requests_pathname_prefix must be set manually. For greater parity with Dash for Python, the following route-related variables should be checked before setting route prefixes to NULL:

~~DASH_URL_BASE_PATHNAME~~
DASH_ROUTES_PATHNAME_PREFIX
DASH_REQUESTS_PATHNAME_PREFIX

Two additional helper functions are proposed in this PR:

get_package_mapping: given path to dependency, package string from request URL, and list of dependencies, attempts to build a package_map. The mapping is then used to resolve local paths to dependencies within the R packages that provide them. A list is returned with the R package name providing the dependency, as well as a local path to the file.
get_mimetype: resolve dependency MIME type based on file extension, currently supports application/JavaScript, application/json, text/css.

Lastly, previous versions of DashR did not include a _dash-component-suites handler. This PR also

eliminates static handling of routes for Dash components served upon index loading using ressource_route (which resolves a duplicate route error for the React JS upon relaunching an existing DashR app in the current workspace)
related to item above, 🔪 dashR::resolve_dependencies and register_dependencies
constructs script tags manually, rather than with htmltools
provides a dash_suite handler to serve dependency files, following the Dash convention
provides support for inserting the DashR app name into dependency pathnames
provides support for including a Cache-Control header, and setting the cache lifetime via components_cache_max_age
more careful CSS handling
treats dependencies bundled into DashR (i.e. dash_renderer, react, react-dom) as being part of the dash_renderer "package", to mirror behaviour in Dash for Python
some basic sanity checking for dependencies: removing multiple slashes in pathnames, ensuring that source maps aren't being served until user interacts with debugger, checking for duplicate dependencies

This PR is a work in progress, aiming to implement similar behaviour as found in _configs.py:

https://github.com/plotly/dash/blob/f590166be2ca476c8970512fd9b23aaf26541ae8/dash/_configs.py#L8

...and in dash.py:

https://github.com/plotly/dash/blob/1249ffbd051bfb5fdbe439612cbec7fa8fff5ab5/dash/dash.py#L488

Closes #52.

chriddyp · 2019-02-21T15:58:43Z

I think let's 🔪 DASH_URL_BASE_PATHNAME - We've only included it for backwards compatible reasons in dash.py. Instead, we can force users to be more explicit by setting both requests and routes prefixes

chriddyp · 2019-02-21T15:59:33Z

R/dash.R

@@ -268,7 +268,14 @@ Dash <- R6::R6Class(
        TRUE
      })

-
+      route$add_handler('get', '/*', function(request, response, keys, ...) {


Did you have to move this around so that it gets declared after the other routes or does it work like this?

Oh nvm, the original route wasn't removed: https://github.com/plotly/dashR/pull/53/files#diff-1881af3720f8f49fd3a44e51c9fb5a0dR180

Let's 🔪 the original route since /* should catch /.

Also, this route needs to be prefixed by routes_pathname_prefix

Right, I noticed that when I glanced at the Python code but forgot to insert here.

fixed in 62d7dd9

chriddyp · 2019-02-21T16:01:22Z

R/dash.R

@@ -370,8 +377,32 @@ Dash <- R6::R6Class(
    # private fields defined on initiation
    name = NULL,
    serve_locally = NULL,
-    routes_pathname_prefix = NULL,
-    requests_pathname_prefix = NULL,
+    routes_pathname_prefix = {


This syntax is interesting - is this like a function that gets called whenever this variable is accessed? Or is it just a function shorthand?

I realized this isn't going to work the way we want, so I've 🔨 a bit, hopefully the refactored implementation of the routes/requests pathname prefixes is closer to the Python logic.

chriddyp · 2019-02-21T16:07:10Z

R/dash.R

-    routes_pathname_prefix = NULL,
-    requests_pathname_prefix = NULL,
+    routes_pathname_prefix = {
+      routes_prefix <- Sys.getenv("DASH_ROUTES_PATHNAME_PREFIX")


These variables are also defined in the constructor:

routes_pathname_prefix = '/', requests_pathname_prefix = '/',

The order of preference should be:

Did the user supply it in the constructor? If so, then use that.

If not, then check the environment

If not in the environment, then use the default.

In Python, for (1), the only way to check if a user supplied it while keeping a default argument was to set that default argument to None. That way, if it's not None, then we can assume that the user set it. (If the default was /, then we wouldn't know whether the user explicitly supplied that or if its just the default).

OK, perfect. This makes sense. I’ll just use NULL instead of None.

fixed in 5fba10f

rpkyle · 2019-02-21T18:38:59Z

I think let's 🔪 DASH_URL_BASE_PATHNAME - We've only included it for backwards compatible reasons in dash.py. Instead, we can force users to be more explicit by setting both requests and routes prefixes

fixed in 5fba10f

…ent-suites

…etadata)

- parses the request URL to identify the dependency that should be returned - calls get_package_mapping to determine the local path to the requested dependency, and the name of the R package which contains it - returns UTF-8 encoded version of dependency from local package source - calls get_mimetype to determine the correct MIME type for the dependency being returned, based on its file extension

…L if not

rpkyle · 2019-03-07T22:36:55Z

Previously, we encountered an error: subscript out of bounds message in the R console when attempting to pass a NULL value to the callback handler. This caused issues with _dash-update-component.

This is possible when using a component like coreLocation, in which an ID may be supplied without a default value. The original code:

callback_args <- lapply(c(request$body$inputs, request$body$state), `[[`, 3)

...has been replaced with:

dashR/R/dash.R

Lines 244 to 260 in 7fa2581

    
           callback_args <- list() 
        
           for (input_element in request$body$inputs) { 
        
             if(is.null(input_element$value)) 
        
               callback_args <- c(callback_args, list(list(NULL))) 
        
             else 
        
               callback_args <- c(callback_args, input_element$value) 
        
           } 
        
           if (length(request$body$state)) { 
        
             for (state_element in request$body$state) { 
        
               if(is.null(state_element$value)) 
        
                 callback_args <- c(callback_args, list(list(NULL))) 
        
               else 
        
                 callback_args <- c(callback_args, state_element$value) 
        
             } 
        
           }

The new code handles inputs which may contain NULL values, which we wish to retain. In R, assigning NULL into a list element removes the element from a list, while appending NULL inserts into an existing list.

https://cran.r-project.org/doc/FAQ/R-FAQ.html#Others:

rpkyle · 2019-03-07T22:42:44Z

I've tried to address the most recent round of PR comments, and I think I've licked the _dash-update-component error Nadia mentioned. If it's possible to review and consider 💃 this one, I'm ready to tackle other issues 🙂

@chriddyp @alexcjohnson @NadiaTahiri @nicolaskruchten

chriddyp · 2019-03-08T21:08:32Z

R/internal.R

+                                                      version = "16.2.0",
+                                                      src = list(href = "https://unpkg.com/react@16.2.0",
+                                                                 file = "lib/react@16.2.0"),
+                                                      meta = NULL,


It looks like this is just white space removal, but a few things strike me as odd about this code:

"react-prod" is react-16. This is different than the python side - we allow users to switch to react 16 optionally but it's not a "prod" vs "dev" distinction.

On the python side, we handle this in dash-renderer using the same structure as the rest of the component repos (https://github.com/plotly/dash-renderer/blob/1c7c75d3127a742093f59bd34ea23b9a84be1ac9/dash_renderer/__init__.py). I would be worried about DashR handling these versions independently of dash-renderer as we don't want DashPy to be distributed with a different version than DashR

This is non-blocking and not a high priority, but something to maybe break out into its own issue and consider down the line.

I should add a debug=TRUE option to DashR, so that component developers can optionally request specific (development) versions of dependencies, replicating functionality currently available in Dash for Python.

This item is on the roadmap for DashR, but I'll open an issue more clearly outlining what's required, and link here for future reference.

chriddyp · 2019-03-08T21:12:03Z

R/utils.R

+                       "/",
+                       paste(dep$src$file,
+                             dep$script,
+                             sep = "/")


If this is only used as a file path, could we use file.path? I recognize that it's being used below, but just to get into the habit of never constructing file paths by hand.

(unless this variable is also used as a script tag / url pathname? By reading the code below it didn't seem like it but I might've missed something)

fixed in 3a61d02

chriddyp · 2019-03-08T21:12:45Z

R/utils.R

+      modified <- as.integer(Sys.time())
+    }
+
+    # we don't want to serve the JavaScript source maps here


maybe add a comment like "until we have full support for a debug=TRUE mode"?

Good call. I could open an issue for this as well, I'll have a look at what's needed to approach parity based on what's currently available in Dash for Python.

fixed in b6af99b

chriddyp · 2019-03-08T21:19:12Z

R/utils.R

+                      sep = "/")
+
+    # remove n>1 slashes and replace with / if present


could you prefix this comment with something like "htmltools sometimes includes multiple // - replace these with /".
That way, the reader knows that // is coming from htmltools and not necessarily the dash-renderer API. In the future if you refactor out htmltools, then the reader will know that this // won't need to be handled because htmltools was the source of the issue.

I've started the process of 🔪 htmltools, managed to remove nearly all references to this package, but not quite there yet. I opened an issue earlier this week which attempts to enumerate all the spots in the code and comments that are involved:

#54

Yeah - My suggestion here is mostly just to get in the habit of writing these types of commits - if there is something in the code that looks odd, it's good to get in the habit of explaining why we have to do stuff like this and where the problem originated.

In this case, everyone involved in this PR is now aware of the issue and you will be the one refactoring it shortly. However, imagine the case where this refactor might happen 6 months from now by someone else. If they didn't know that this // was originating from htmltools, then they might not feel comfortable removing this block of code. Perhaps they might think that dash-renderer (a 3rd party piece of code) sometimes introduces a // in certain scenarios, scenarios that might not necessarily be tested in integration tests.

So, it's not strictly necessary in this scenario where we plan on fixing this up in a couple of weeks, but it's a good habit to get into and a healthy way to start thinking about comments

For sure, decisions like these seem apparent now that I'm staring at the same code over and over again, but for anyone else (or me even a year from now), statements like this are hard to comprehend in total isolation.

I'll be a bit more diligent about making these sorts of commits, it benefits everyone from the author, to the reviewer, and even savvy users who decide to take a look under the hood.

fixed in b77c94d

chriddyp

Thanks for working through all of these suggestions!

💃 Once you've considered a few small suggestions concerning comments

initial attempt at resolving env vars for route prefixing

0abe05b

rpkyle changed the title ~~[WIP] Inspect environmental variables and set routing prefixes as needed~~ [WIP] Inspect environment variables and set routing prefixes as needed Feb 21, 2019

rpkyle changed the title ~~[WIP] Inspect environment variables and set routing prefixes as needed~~ [WIP] Inspect environment variables and set routing prefixes, add catchall endpoint Feb 21, 2019

rpkyle requested review from alexcjohnson, chriddyp and NadiaTahiri February 21, 2019 15:45

rpkyle self-assigned this Feb 21, 2019

rpkyle added this to the DashR Launch milestone Feb 21, 2019

chriddyp reviewed Feb 21, 2019

View reviewed changes

Ryan Patrick Kyle added 2 commits February 21, 2019 13:16

✨ catchall handler inserted

1d21afc

🔨 implement same logic for routes/requests resolution as in 🐍

5fba10f

rpkyle force-pushed the 0.0.2-issue52 branch from ffc7f13 to 5fba10f Compare February 21, 2019 18:16

catchall handler now takes a prefixed wildcard

62d7dd9

rpkyle force-pushed the 0.0.2-issue52 branch from f56385f to 62d7dd9 Compare February 21, 2019 18:35

Ryan Patrick Kyle added 12 commits February 27, 2019 11:08

✨ add get_mimetype fn to resolve MIME types by file ext

e03ecdf

✨ added get_package_mapping fn to resolve URL pkg with R pkg name

86fc078

🔨 render_dependencies fn refactored to inject prefix and _dash-compon…

caea954

…ent-suites

🔪 resolve_dependencies shim function

f5c38d2

✒️ inserted whitespace for get_mimetype

df897e5

🔨 use self$config$routes_pathname_prefix

6c90c0a

💄 whitespace removed or inserted

3c82629

🔨 call htmltools::resolveDependencies directly

88c4ef0

✨ dependencies_get_internal fn added (react/react-dom/dash-renderer m…

6980962

…etadata)

✨ added logic for Cache-Control header and components_cache_max_age arg

6d38c76

🔕 added warn=FALSE to silence missing final EOL error

62cc399

Ryan Patrick Kyle added 8 commits March 5, 2019 12:13

🔨 improve handling of CSS dependencies, either by file or href

cfa81f6

💡 add comments to explain the input/output params of get_package_mapping

bea8ffc

🐛 ensure dependencies are not duplicated before building package_map

967cdba

🐛 check if package name exists in get_package_mapping, and return NUL…

60bd516

…L if not

🔍 add check for n>1 slashes, 💡 comment code

3822183

🔪 cast to data frame, simplify to table transpose of matrix only

b34c4f8

🐫 remove repeated call to basename

37bf706

🔪 simplify code, just call file_ext instead

0f2e084

rpkyle force-pushed the 0.0.2-issue52 branch from d2630fa to 0f2e084 Compare March 5, 2019 17:28

Ryan Patrick Kyle added 3 commits March 5, 2019 12:59

🐛 should be && instead of & in case script does not exist

1dbb46a

👌 added comments to explain pos_match

8362cb5

🐛 fix issue with initial NULL value handling in _dash-update-component

7fa2581

rpkyle requested a review from chriddyp March 7, 2019 22:38

rpkyle added 3 commits March 8, 2019 12:27

⏪ drop required R version to 3.0.2

2a7831d

Bump DashR version to 0.0.3

c301da4

⏪ downgrade required R version in README.md

e25b9a9

chriddyp reviewed Mar 8, 2019

View reviewed changes

chriddyp approved these changes Mar 8, 2019

View reviewed changes

Ryan Patrick Kyle added 3 commits March 8, 2019 17:23

👌 use file.path for platform-independent path specification

3a61d02

📝 added comment about debug mode

b6af99b

👌 added comment explaining slash removal code

b77c94d

rpkyle merged commit 14972f4 into master Mar 9, 2019

This was referenced Mar 11, 2019

Address bug in remote CSS dependency handling #56

Closed

Support external_stylesheets and handle remote CSS as in Dash for Python #59

Merged

rpkyle deleted the 0.0.2-issue52 branch November 4, 2019 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Inspect environment variables and set routing prefixes, add catchall endpoint #53

[WIP] Inspect environment variables and set routing prefixes, add catchall endpoint #53

rpkyle commented Feb 21, 2019 •

edited

Loading

chriddyp commented Feb 21, 2019

chriddyp Feb 21, 2019

chriddyp Feb 21, 2019

rpkyle Feb 21, 2019

rpkyle Feb 21, 2019

chriddyp Feb 21, 2019

rpkyle Feb 21, 2019

chriddyp Feb 21, 2019

rpkyle Feb 21, 2019

rpkyle Feb 21, 2019

rpkyle commented Feb 21, 2019

rpkyle commented Mar 7, 2019 •

edited

Loading

rpkyle commented Mar 7, 2019

chriddyp Mar 8, 2019

rpkyle Mar 8, 2019 •

edited

Loading

chriddyp Mar 8, 2019

rpkyle Mar 8, 2019

chriddyp Mar 8, 2019

rpkyle Mar 8, 2019

rpkyle Mar 8, 2019

chriddyp Mar 8, 2019

rpkyle Mar 8, 2019

chriddyp Mar 8, 2019

rpkyle Mar 8, 2019

rpkyle Mar 8, 2019

chriddyp left a comment

		sep = "/")

		# remove n>1 slashes and replace with / if present

[WIP] Inspect environment variables and set routing prefixes, add catchall endpoint #53

[WIP] Inspect environment variables and set routing prefixes, add catchall endpoint #53

Conversation

rpkyle commented Feb 21, 2019 • edited Loading

chriddyp commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpkyle commented Feb 21, 2019

rpkyle commented Mar 7, 2019 • edited Loading

rpkyle commented Mar 7, 2019

Choose a reason for hiding this comment

rpkyle Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriddyp left a comment

Choose a reason for hiding this comment

rpkyle commented Feb 21, 2019 •

edited

Loading

rpkyle commented Mar 7, 2019 •

edited

Loading

rpkyle Mar 8, 2019 •

edited

Loading