-
-
Notifications
You must be signed in to change notification settings - Fork 31
Dash for R v0.5.0 #205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dash for R v0.5.0 #205
Conversation
* rename pruned_errors to prune_errors * rename pruned to prune * provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors * rename pruned to prune * 🔨 handle stop errors * 🚨 add test for stop errors
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133) * ✨ Support line #s when in debug mode * ✨ Add use_viewer option for RStudio * 🚨 Add soft and hard hot reloading tests
add always
* ✨ initial support for meta tags * support arbitrary tags * 🚨 add tests * 🔬 add asserts * add reference to meta tag PR * ⏩ indent meta tags
* add eager_loading parameter * 📛 add buildFingerprint * 📛 add checkFingerprint * use getDependencyPath, + 🐾/Etag support * updates to support async * ✨ properly support gz compression * 🐛 post-async fixes for CSS handling
…races (#137) * 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces * 🚚 add polyfill.js
* refactor resolvePrefix * camel case resolve_prefix * Update R/utils.R Co-authored-by: HammadTheOne <[email protected]>
* 🐛 support composite classes w/ updatable_outputs * 🚨 add test for callback returning component
…121) * remove is.layout + layout_container_id * remove container test * refactor to require component/tree of components * fix unit test * fix duplicated ID handling * update unit test, add another * fix wildcards test
josegonzalez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just come general feedback.
| - Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204) | ||
|
|
||
| ### Changed | ||
| - Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josegonzalez Good question; ultimately, it means that code like this:
app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:
app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.
I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.
| echo "RUNNING JOB: ${CIRCLE_JOB}" | ||
| echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}" | ||
| echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}" | ||
| export PERCY_ENABLE=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add this as an enhancement :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.
| ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids) | ||
| props <- lapply(names(map), function(x) dash:::getIdProps(x)$props) | ||
| ids <- lapply(names(map), function(x) getIdProps(x)$ids) | ||
| props <- lapply(names(map), function(x) getIdProps(x)$props) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the namespace removal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.
What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).
Very much a 🙈 moment.
| } | ||
|
|
||
| validate_keys <- function(string) { | ||
| validate_keys <- function(string, is_template) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the signature change impact existing calls to validate_keys that only have a single argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josegonzalez validate_keys is an internal function, and only called from two Dash methods:
interpolate_indexindex_string
The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.
Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.
| ) | ||
| }) | ||
|
|
||
| test_that("Customizing title using `name` produces a warning", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we raise an error now instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.
[0.5.0 ] - 2020-05-28
Added
brotlipackage explicitly; previously it was loaded when importingreqres. #204Changed
htmlDivinternally, for parity with Dash for Python. Starting in v0.5.0, thelayoutmethod only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121roxygen2when documenting R6 classestitlemethod now specifiesDashas the default application title instead ofdash. #200Fixed
validate_keyswhich preventedinterpolate_indexfrom working as intended has been resolved