Skip to content

Remove dependencies_* methods in Dash class#74

Merged
rpkyle merged 5 commits into
masterfrom
0.0.4-issue65
Apr 9, 2019
Merged

Remove dependencies_* methods in Dash class#74
rpkyle merged 5 commits into
masterfrom
0.0.4-issue65

Conversation

@rpkyle

@rpkyle rpkyle commented Mar 31, 2019

Copy link
Copy Markdown
Contributor

This PR proposes to relocate remove three methods:

  • dependencies_set
  • dependencies_get
  • dependencies_get_internal

to the private field of the Dash R6 class. Closes #65.

@rpkyle rpkyle self-assigned this Mar 31, 2019
@rpkyle rpkyle requested a review from TahiriNadia March 31, 2019 04:24
@rpkyle

rpkyle commented Mar 31, 2019

Copy link
Copy Markdown
Contributor Author

@TahiriNadia If you could take a look, would be grateful. Thanks in advance! 🙂

@rpkyle

rpkyle commented Apr 7, 2019

Copy link
Copy Markdown
Contributor Author

@TahiriNadia Please review this PR, and confirm that removing these methods does not produce any errors or undesirable behaviour upon loading DashR apps. It should not cause any issues, but we'll need to be certain. Thanks 🙂

@TahiriNadia

Copy link
Copy Markdown
Contributor

The update gives me an error

Error in eval(parse(text = example.ready.for.eval)) :
  attempt to apply non-function
Calls: source ... withVisible -> eval -> eval -> <Anonymous> -> eval -> eval
Execution halted

@TahiriNadia

Copy link
Copy Markdown
Contributor

eval(parse(text=example_string)) is used in utils.R file

LoadAndDisplayComponent <- function(example_string) {
  return(htmlDiv(list(
    htmlDiv(
      children=dccSyntaxHighlighter(example_string),
      className='code-container'
    ),
    htmlDiv(
      className='example-container',
      style = list("overflow-x" = "initial"),
      children=eval(parse(text=example_string))
    )
  )))
}

@rpkyle

rpkyle commented Apr 9, 2019

Copy link
Copy Markdown
Contributor Author

eval(parse(text=example_string)) is used in utils.R file

@TahiriNadia The utils.R script is not part of the DashR package; there are no eval(parse calls in DashR itself.

Could you try again, but instead of the DashR docs, try a simpler test application or two? Remember that this branch is old enough that it still uses layout_set instead of layout.

I just re-ran my code, and there are no issues on my side when removing these methods (they aren't essential for writing a basic app, so it makes sense). Thanks in advance for your help.

@TahiriNadia

Copy link
Copy Markdown
Contributor

@rpkyle I cloned again the dashR repo and installed it. It seems working, but when I checked the dashR::Dash in R console it gives me this info (see below). I didn't find:

  • dependencies_set ?
  • dependencies_get ?
  • dependencies_get_internal ?
> dashR::Dash
<Dash> object generator
  Public:
    server: NULL
    config: list
    initialize: function (name = "dash", server = fiery::Fire$new(), assets_folder = "assets", 
    layout_get: function (render = TRUE) 
    layout: function (...) 
    react_version_set: function (version) 
    callback: function (output, params, func) 
    run_server: function (host = NULL, port = NULL, block = TRUE, showcase = FALSE, 
    run_heroku: function (host = "0.0.0.0", port = Sys.getenv("PORT", 8080), 
    clone: function (deep = FALSE) 
  Private:
    name: NULL
    serve_locally: NULL
    assets_folder: NULL
    assets_url_path: NULL
    assets_ignore: NULL
    routes_pathname_prefix: NULL
    requests_pathname_prefix: NULL
    suppress_callback_exceptions: NULL
    asset_map: NULL
    css: NULL
    scripts: NULL
    other: NULL
    dependencies: list
    dependencies_user: list
    dependencies_internal: list
    layout_: NULL
    layout_ids: NULL
    callback_map: list
    .index: NULL
    layout_render: function () 
    walk_assets_directory: function (assets_dir = private$assets_folder) 
    componentify: function (x) 
    react_version_enabled: function () 
    react_deps: function () 
    react_versions: function () 
    collect_resources: function () 
    index: function () 
  Parent env: <environment: namespace:dashR>
  Locked objects: TRUE
  Locked class: FALSE
  Portable: TRUE

@rpkyle rpkyle changed the title Relocate dependencies_* methods to private field of Dash class Remove dependencies_* methods in Dash class Apr 9, 2019
@rpkyle

rpkyle commented Apr 9, 2019

Copy link
Copy Markdown
Contributor Author

I think I see what's happening here. The methods have been 🔪, but the docs need to be regenerated to reflect this.

I believe it makes sense to 🔪 these, they aren't necessary anymore -- we should use assets, external_stylesheets, and external_scripts to serve dependencies, as this is idiomatic for Dash in Python as well.

Since I'm not using this API either, it feels a bit pointless to privatize it at this point. This is the old way, we're pretty close to abandoning htmltools altogether, and it seems likely they'll eventually fail. My plan post-release is to refactor dependency management to more closely mimic the Python side of things anyhow.

@alexcjohnson @nicolaskruchten May I excise these methods ☝️ ?

@alexcjohnson

Copy link
Copy Markdown
Collaborator

Yes, 🔪

@TahiriNadia

Copy link
Copy Markdown
Contributor

I agree 🔪 if not needed. You can 💃

@rpkyle rpkyle merged commit 8d17466 into master Apr 9, 2019
@rpkyle rpkyle deleted the 0.0.4-issue65 branch April 9, 2019 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants