Dash R Core Package Unification

[HammadTheOne]

This PR addresses plotly/dash-core#283 by introducing a gulpfile based script in the root directory of the dashr repo. This script sources from gulp-assets, which contains the main, updated, R export files from the respective core packages (dash-core-components, dash-html-components, and dash-table).

Running the npm unify command executes a series of tasks which incorporate the core package dependencies and NAMESPACE exports to the dash package, in preparation for a CRAN release. Running the npm clean command restores these dependencies so that the package can be used independently off of GIthub with the other core packages.

• Fix DESCRIPTION to remove empty lines when running unify.
• Run Rscript -e 'devtools::document()' to populate documentation help index.
• Possibly remove all artifacts in gulp-assets to reduce package size until needed.
• Open PR's in dcc and html to remove library imports from vignettes and examples.

Closes plotly/dash-core#283.

[rpkyle]

This is a great start -- thanks for taking this project on, definitely not the low-hanging fruit but this will make maintaining the package far easier, and dramatically simplify installations and version control for users when upgrading! 🙌

I think we have a bit more to puzzle through here before we proceed. Here's a list of the initial items to resolve:

• Do away with gulp-assets, so we 🚁 relocate the artifacts we want to retain from the three core packages directly into the cloned dash repo, which should simplify cleanup also
• Remove the three package-specific .Rd files like dashCoreComponents-package.Rd before merging
• Edit dash-info.yaml to scrub references to dashTable, dashHtmlComponents, dashCoreComponents
• Remove && require(dash) from the example apps, since there won't be issues with loading the component libraries without dash anymore 😁
• Avoid hardcoding version numbers in the gulpfile.js (I suggested some regex, but there are multiple ways around this, the preferred strategy is up to you)
• Double-check that the unified package handles pathname resolution properly for the merged-in package assets (there's a comment about this, maybe I'm paranoid, I'll test myself also)

Once we've sorted out ☝️, we should probably consider a deprecation message to appear conditionally on startup if any of the libraries are attached that are now removed. It seems doable without much fuss by storing .packages() and then checking whether the libraries were previously loaded, e.g. something like

> library(dashHtmlComponents)
> library(dashCoreComponents)
> library(dash)
NOTE: As of version 1.0, the following packages are deprecated and should no longer be installed or loaded when using Dash for R: dashHtmlComponents, dashCoreComponents, dashTable. These components are now bundled into the dash package.

... and then let's 💃 🕺 🚀

• bump dash version to 1.0 since the API and package loading syntax is now stable!

[rpkyle]

Was the creation and commission of the gulp-assets folder intentional? If it's possible to relocate the resources in-place once, that would be ideal (e.g. things destined for R, man, inst are copied into dash as needed and artifacts are committed only once to their final destination).

[HammadTheOne]

The main point of having a dedicated directory was for ease of development and updates - for any DashR releases or version updates for the component packages, we can simply copy over the existing component directories to gulp-assets and run the update script to move the assets within to their respective dependency directories.

I do want to take another pass over this and maybe make it a little more like the dash build process so there's less relocation.

[rpkyle]

Hmm, my gut feeling is that we probably want to retain the vestigial package value when serializing into JSON, even though dashCoreComponents technically no longer exists.

In other words, right now this is the behaviour, and I'm guessing it should remain as-is:

> library(dash)
> library(dashHtmlComponents)
> htmlDiv()
{
  "props": {},
  "type": "Div",
  "namespace": "dash_html_components",
  "propNames": ["children", "id", "n_clicks", "n_clicks_timestamp", "key", "role", "accessKey", "className", "contentEditable", "contextMenu", "dir", "draggable", "hidden", "lang", "spellCheck", "style", "tabIndex", "title", "loading_state"],
  "package": "dashHtmlComponents"
} 

@alexcjohnson What are your thoughts? Should we retain the existing namespace and package values? I imagine the namespace must stay the same, but should we update package to be dash now?

[rpkyle]

@HammadTheOne Given the reasons outlined in #243 (comment), I believe we'll need to retain the package key and update it to reflect the migration of the underlying JS into the dashpackage.

So the JSON returned here should look like this, since the namespace will remain the same, but package will change (so R can identify the proper system path when Dash serves up the asset to the browser):

{
  "props": {},
  "type": "Div",
  "namespace": "dash_html_components",
  "propNames": ["children", "id", "n_clicks", "n_clicks_timestamp", "key", "role", "accessKey", "className", "contentEditable", "contextMenu", "dir", "draggable", "hidden", "lang", "spellCheck", "style", "tabIndex", "title", "loading_state"],
  "package": "dash"
} 

[rpkyle]

Similarly here, for the "Imports" line I think we might want to use a regex to scrub the old package entries. This would enable us to avoid hardcoding version numbers.

Just as an example, I believe I wrote a pattern that might help here when I automated the Dash build tests for R:

dashR/tests/circleci/fixup_metadata.R

Lines 30 to 32 in edbcc04

	remotes <- gsub("((?<=plotly\\\\/dash-html-components@)([a-zA-Z0-9]+))", dhc_hash, remotes, perl=TRUE)
	remotes <- gsub("((?<=plotly\\\\/dash-core-components@)([a-zA-Z0-9]+))", dcc_hash, remotes, perl=TRUE)
	remotes <- gsub("((?<=plotly\\\\/dash-table@)([a-zA-Z0-9]+))", dt_hash, remotes, perl=TRUE)

Keep in mind that some of the characters are for escaping other characters and I haven't tested this pattern outside of R 🙈

[alexcjohnson]

Do we need to be operating on DESCRIPTION with gulp at all? That file isn't auto-generated AFAICT, and the changes we're making here don't depend on anything variable coming in from the components. Can't we just make these changes by hand and leave them alone?

[rpkyle]

@HammadTheOne I'm with @alexcjohnson here, let's avoid editing DESCRIPTION with Gulp.js, we build this package "by hand" anyhow.

[rpkyle]

Not entirely sure, but I suspect some test failures may have occurred as a result of now-deprecated packages being loaded. Removed those in ef6a5c7, will try to get those passing before we merge.

[rpkyle]

Not entirely sure, but I suspect some test failures may have occurred as a result of now-deprecated packages being loaded. Removed those in 6ee0633, will try to get those passing before we merge.

In addition to the failing integration tests, we were seeing some failing unit tests, so I've fixed those also in ef6a5c7.

[rpkyle]

✨

[rpkyle]

🎉

[rpkyle]

Did this get removed when you re-ran devtools::document()? The collation order of the R source files may not be important, but if we remove the entry (or devtools did) we should just ensure that no issues arise from the new sourcing order.

[HammadTheOne]

I did remove this manually. Ideally what I would expect to happen is that all the new .R files present in the package like htmlHeader.R would be appended to the Collate entry with roxygen2::oxygenise() . However, when I execute this, it does not generate an updated DESCRIPTION and instead reverts the dependencies in NAMESPACE instead. If we have the Collate entry present, all of the R files must be included or installation of the package fails.

Do you think there's a better way around it than simply removing it? I didn't notice any issues arising from removing it myself.

[rpkyle]

@HammadTheOne Collate probably doesn't matter unless there are potential side effects, and apparently the most common cause of this is use of S4 methods, which we don't have in this package:

https://r-pkgs.org/description.html#:~:text=Collate%20controls%20the%20order%20in,access%20data%20in%20your%20package.

I think it's safe to remove it. If the tests pass, and the package functionality remains as before, I'm OK if we 🔪 this block.

[rpkyle]

The script tags unit test was failing because the metadata for the JavaScript assets is no longer provided by external packages, so I've updated it also in 227df58. I'm optimistic that this should address the remaining test issues following the unification.

[alexcjohnson]

🔪 (I made a similar comment above, are there more cases like this?)

[alexcjohnson]

Yes, for example man/dash.Rd has a bunch

[HammadTheOne]

I've updated it in d66d720 for this branch. However, for future dcc/html/table updates, we'll also need to make PR's in those repos once this PR is merged to update the dash-info.yaml and scrub the library imports and require(dash) from the examples. It should be a relatively minor update and I can open those PR's after this one is merged.

[HammadTheOne]

@ryan @alexcjohnson I think this PR should be in pretty good shape with these last few commits. CI tests are running successfully (thanks @rpkyle!) and I haven't encountered any obvious issues in running multiple test apps. There should be no more reference to imports of the component libraries, and I've added in some gulp tasks that should make updating those libraries within DashR in the future a relatively easier process. There's still a couple of minor things like writing up a CONTRIBUTING.md or at least an internal wiki on the update process, and opening PR's to scrub the dash-info.yaml in the other repos, but I think those can be addressed after merging this one.

I'd love to know if you have any final feedback before we can close this one out 🙂

[rpkyle]

Random note: looks like the example URL for htmlSource and htmlTrack has changed from

https://interactive-examples.mdn.mozilla.net/media/examples/friday.mp4

to

https://interactive-examples.mdn.mozilla.net/media/cc0-videos/friday.mp4

[rpkyle]

@HammadTheOne I went through the process of running the full Gulp workflow, assembled the unified package, and was able to run an application successfully -- but not without this favicon.ico error:

error: argument is of length zero
request: 127.0.0.1 - ID_127.0.0.1 [18/May/2021:00:08:03 -0400] "GET /_favicon.ico HTTP/1.1" 500 0 "http://127.0.0.1:8050/" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36"

I think this default favicon.ico tag was added in #240, to handle the case where a favicon.ico is not provided by an app. I see the one your PR added to inst, but I don't think it's loading here given the error above:

<link href="/_favicon.ico" rel="icon" type="image/x-icon">

We should fix this before we merge, I think (assuming you can reproduce this also).

[HammadTheOne]

We should fix this before we merge, I think (assuming you can reproduce this also).

Thanks for catching that 🐛 - based on our discussion, this is now fixed in ec1ab35.

[rpkyle]

@HammadTheOne Thanks for the additional favicon fix, looks 👌 on my end. I took the liberty of editing gulpfile.js to use a shallow repo clone to speed things up when rebuilding the package. 🏆 💃

I'll see if I can fix up the dash-info.yaml in the core repos myself and then we can rebuild this again before the release with updated examples.

[alexcjohnson]

Let's do it! 💃