- Notifications
You must be signed in to change notification settings - Fork85
Standardize single doc deployment#698
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
R/deployApp.R Outdated
| if (dirExists(appDir)) { | ||
| # ok | ||
| }elseif (file.exists(appDir)&& isStaticFile(appDir)) { | ||
| doc<- standardizeSingleDocDeployment(appDir) |
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.
One thing that's slightly unappealing about this approach is that we use a different approach to determiningappFiles depending on whether you dodeployApp("foo.Rmd") ordeployApp(appPrimaryDoc = "foo.Rmd"). An alternative approach would be to add anappPrimaryDoc argument toappStandardizeFiles() and callrmarkdown::find_external_dependencies() there whenappPrimaryDoc is supplied.
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.
Another approach, since this never appears to have been documented, would be to deprecate this useappDir and tell folks to calldeployDoc() 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.
Interesting. I forgot about this difference.
# Uses the default appDir=getwd() and enumerates all files in the directory for bundlingdeployApp(appPrimaryDoc="foo.Rmd")# Uses the foo.Rmd to identify the dependencies of the document; only those are bundleddeployDoc("foo.Rmd")
The existingdeployApp("foo.Rmd") feels like a convenience, but we should nudge folks towards usingdeployDoc should they provide a file.
Added in:#16, and before the notion of a "primary doc".
I'm uneasy about the automatic use ofrmarkdown::find_external_dependencies. On the one hand, it does feel like it will produce the set of files that most folks want to include -- directory explosion is often too greedy. On the other hand, its use would add to the "magic" that happens in the "lower-level"deployApp. Its inclusion could suddenly force more folks to compute their ownappFiles.
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.
Deprecating thedeployApp -> deployDoc -> deployApp workflow feels like the right path.
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.
Yeah, I think the fundamentally different mechanism of file discovery suggests that this is adeployDoc() feature, and we should deprecate this old path.
R/deployApp.R Outdated
| if (dirExists(appDir)) { | ||
| # ok | ||
| }elseif (file.exists(appDir)&& isStaticFile(appDir)) { | ||
| doc<- standardizeSingleDocDeployment(appDir) |
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.
Interesting. I forgot about this difference.
# Uses the default appDir=getwd() and enumerates all files in the directory for bundlingdeployApp(appPrimaryDoc="foo.Rmd")# Uses the foo.Rmd to identify the dependencies of the document; only those are bundleddeployDoc("foo.Rmd")
The existingdeployApp("foo.Rmd") feels like a convenience, but we should nudge folks towards usingdeployDoc should they provide a file.
Added in:#16, and before the notion of a "primary doc".
I'm uneasy about the automatic use ofrmarkdown::find_external_dependencies. On the one hand, it does feel like it will produce the set of files that most folks want to include -- directory explosion is often too greedy. On the other hand, its use would add to the "magic" that happens in the "lower-level"deployApp. Its inclusion could suddenly force more folks to compute their ownappFiles.
R/deployApp.R Outdated
| if (dirExists(appDir)) { | ||
| # ok | ||
| }elseif (file.exists(appDir)&& isStaticFile(appDir)) { | ||
| doc<- standardizeSingleDocDeployment(appDir) |
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.
Deprecating thedeployApp -> deployDoc -> deployApp workflow feels like the right path.
Extract out
standardizeSingleDocDeployment()and use it indeployDoc()anddeployApp().I've also documented this feature of
deployApp(), which lead to some other minor doc changes since other function inherited fromdeployApp(). This does raise a question of whether these functions should also support anappDirthat's actually a path to a document.