Skip to content

Conversation

@rpkyle
Copy link
Contributor

@rpkyle rpkyle commented May 28, 2020

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #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
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyle and others added 30 commits August 23, 2019 09:23
* 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
* ✨ 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]>
Copy link

@josegonzalez josegonzalez left a 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)

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?

Copy link
Contributor Author

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

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

Copy link
Contributor Author

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the namespace removal?

Copy link
Contributor Author

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) {

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?

Copy link
Contributor Author

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_index
  • index_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", {

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?

Copy link
Contributor Author

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.

@rpkyle rpkyle merged commit c0c2563 into master May 29, 2020
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.

6 participants