Skip to content
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

Change default behaviour for R package generator to recycle package version info #870

Closed
rpkyle opened this issue Aug 15, 2019 · 5 comments
Assignees
Milestone

Comments

@rpkyle
Copy link
Contributor

rpkyle commented Aug 15, 2019

Currently, the R package generation code inserts an integer (which increments for each additional asset inserted into the dependency list) for the version number when none is available in __init__.py.

It would be safer to default to inserting the same version information as for the containing package, since this currently requires developer intervention and editing of internal.R after the package is generated. A (fictional) example:

.dashAwesomeExtensions_js_metadata <- function() {
  deps_metadata <- list(`dash_awesome_extensions` = structure(list(name = "dash_awesome_extensions",
                                                                   version = "2.0", src = list(href = NULL,
                                                                                               file = "deps"), meta = NULL,
                                                                   script = 'dash_awesome_extensions.min.js',
                                                                   stylesheet = NULL, head = NULL, attachment = NULL, package = "dashAwesomeExtensions",
                                                                   all_files = FALSE), class = "html_dependency"),
                        `dash_awesome_extensions` = structure(list(name = "dashAwesomeExtensions",
                                                                   version = "1", src = list(href = NULL,
                                                                                             file = "deps"), meta = NULL,
                                                                   script = NULL,
                                                                   stylesheet = 'fancify.css', head = NULL, attachment = NULL, package = "dashDesignKit",
                                                                   all_files = FALSE), class = "html_dependency"),
                        `dash_awesome_extensions` = structure(list(name = "dashAwesomeExtensions",
                                                                   version = "2", src = list(href = NULL,
                                                                                             file = "deps"), meta = NULL,
                                                                   script = NULL,
                                                                   stylesheet = 'stylize.css', head = NULL, attachment = NULL, package = "dashDesignKit",
                                                                   all_files = FALSE), class = "html_dependency"))
  return(deps_metadata)
}

This is important because of the way Dash dependencies are currently fetched via URL:

https://github.com/plotly/dashR/blob/a44050a27ba55056afed8ebac81e852ddf852e1f/R/utils.R#L196-L204

I propose that the default should be to replace version = "1" and version = "2" with version = "2.0" in the example above if version information for dependencies is not available within a generated package.

The relevant code block is here:

# pylint: disable=consider-using-enumerate
if len(alldist) > 1:
for dep in range(len(alldist)):
rpp = alldist[dep]["relative_package_path"]
if "dash_" in rpp:
dep_name = rpp.split(".")[0]
else:
dep_name = "{}".format(project_shortname)
project_ver = str(dep)

@alexcjohnson

@rpkyle rpkyle added this to the Dash v1.3.0 milestone Aug 15, 2019
@rpkyle rpkyle self-assigned this Aug 15, 2019
@alexcjohnson
Copy link
Collaborator

Let's take a step back here - AFAICT this version is just used in the cache-busting query param when we generate a <script> or <link> tag. Right now we include both version and file mod time in this query param ?v={}&m={}. We've had the version for a long time, mod time was added last fall in #387 - along with components_cache_max_age which we've since removed as unnecessary specifically because we have mod time in all the urls.

Is there really any reason we need both of these? Once we have mod time - which supports changes during development as well as users upgrading versions - isn't the version redundant? I propose we simply get rid of version and only use mod time.

@rpkyle
Copy link
Contributor Author

rpkyle commented Aug 15, 2019

From my point of view, I think 🔪 the v= part of the URL makes a lot of sense.

I've yet to run into any generated packages/libraries which contain assets where (a) the asset version ≠ the package version, and (b) it's important to maintain this distinct version information.

@rpkyle
Copy link
Contributor Author

rpkyle commented Aug 15, 2019

We've had the version for a long time, mod time was added last fall in #387 - along with components_cache_max_age which we've since removed as unnecessary specifically because we have mod time in all the urls.

Speaking of which, can I introduce a simple PR to 🔪 components_cache_max_age, targeted at dev?

@alexcjohnson
Copy link
Collaborator

Speaking of which, can I introduce a simple PR to 🔪 components_cache_max_age, targeted at dev?

In dashR you mean? Absolutely!

@rpkyle
Copy link
Contributor Author

rpkyle commented Jan 7, 2020

🎉 This issue was resolved in #1048. Closing!

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

No branches or pull requests

3 participants