ropensci / iheatmapr

Complex, interactive heatmaps in R
https://docs.ropensci.org/iheatmapr
Other
267 stars 35 forks source link

Use the same plotly.js version as r-plotly #88

Closed ahmohamed closed 2 years ago

ahmohamed commented 2 years ago

This PR is to initiate the discussion to update the plotly.js version and suggest using the same version as R-plotly package. I'm not expecting you to merge this, as you may have opinions on how / if you want to do this.

Issue

Currently, iheatmapr interferes with r-plotly because of version conflict. Consider knitting an Rmd with the following chunck:

library(iheatmapr)
library(ggplot2)
library(plotly)

# ggplotly with facets
ggplotly(ggplot(mpg, aes(displ, hwy)) + geom_point() + facet_wrap(.~class))

# Plot iheatmapr
mat <- matrix(rnorm(24), nrow = 6)
annotation = data.frame(gender = c(rep("M", 3),rep("F",3)),
  age = c(20,34,27,19,23,30))
iheatmap(mat, 
  cluster_rows = "hclust",
  cluster_cols = "kmeans", 
  col_k = 3, 
  row_annotation = annotation)
Screen Shot 2021-09-26 at 9 50 19 pm

Of course the ggplotly plot renders correctly when iheatmapr is not included. Besides the incompatibility, using different versions of plotly.js bloats the file size (or increase the bundle size in case of a shiny app).

Downsides of this solution

In terms of incompatibility of iheatmapr with the new version of plotly.js, I couldn't see any during my limited testing. CI tests should clarify if. there are any as well.

The downside to unifying the plotly.js version is that we need to add r-plotly to the imports or at least suggests.

AliciaSchep commented 2 years ago

Thanks for this suggestion -- I think the problem here is that the function from plotly is not exported. I don't think it will pass cran rules to call an internal function from another package. The other part that is not ideal here is depending on the plotly package, and it also means this package can't control when it does upgrades of the plotly.js version

AliciaSchep commented 2 years ago

Perhaps an option would be for iheatmapr to ship its own version of plotly.js (which can also be probably updated) but to enable setting an option to pull a different version. One could then set that option to pull from plotly internals if desired.

AliciaSchep commented 2 years ago

Closing in favor of #92, which I will aim to merge in soon and then follow-up with a cran release. Thanks @ahmohamed for show-casing that updating plotly version seems to work okay.