-
-
Notifications
You must be signed in to change notification settings - Fork 977
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
shinyrmd: Safer dependency extraction from pre-rendered HTML #2500
Conversation
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.
This looks good to me! Thanks for this. It makes total sense to do that.
I'd like to know why shiny_prerendered_append_dependencies() is called twice during pre-render
I tried to look into that, but I don't see when we would call render()
another time during the same render. shiny_prerendered_append_dependencies()
is supposed to append to a file, and this file is supposed to be the knitted file so inermediary results. it would require writing another time to that file and I don't see when we would do that. In general case the intermediary file will be overwritten to a fresh version at each knit()
so each render()
.
So it coulb another render call on the intermediary file but I have check our code base and I don't see that.
I suggest we merge this and this will remain a Mystery
[skip ci]
* rstudio/main: start the next version CRAN release v2.24 shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500) quote the version number per CRAN's request Add output_format_dependency() (rstudio#2462) file_scope is now correctly merged when creating output_format (rstudio#2488) Correctly run some tests only on CI start the next version CRAN release v2.23 remove broken links suggest cleanrmd for e499bf7 add news comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548 `find_external_resources` works with custom format using `theme` (rstudio#2494) start the next version CRAN release v2.22 S3 generic/method consistency Change the code-folding button text from "Code" to "Show" (rstudio#2489) fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477) detecting external resources needs to consider css argument (rstudio#2486)
Merge remote-tracking branch 'rstudio/main' into jg-devel # By Yihui Xie (13) and others # Via Yihui Xie * rstudio/main: start the next version CRAN release v2.24 shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500) quote the version number per CRAN's request Add output_format_dependency() (rstudio#2462) file_scope is now correctly merged when creating output_format (rstudio#2488) Correctly run some tests only on CI start the next version CRAN release v2.23 remove broken links suggest cleanrmd for e499bf7 add news comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548 `find_external_resources` works with custom format using `theme` (rstudio#2494) start the next version CRAN release v2.22 S3 generic/method consistency Change the code-folding button text from "Code" to "Show" (rstudio#2489) fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477) detecting external resources needs to consider css argument (rstudio#2486) # Conflicts: # DESCRIPTION
* jg-devel: (21 commits) Updated NEWS. Patched `merge_output_format_dependency` to ensure that named elements remain in the correct order. start the next version CRAN release v2.24 shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500) quote the version number per CRAN's request Add output_format_dependency() (rstudio#2462) file_scope is now correctly merged when creating output_format (rstudio#2488) Correctly run some tests only on CI start the next version CRAN release v2.23 remove broken links suggest cleanrmd for e499bf7 add news comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548 `find_external_resources` works with custom format using `theme` (rstudio#2494) start the next version CRAN release v2.22 S3 generic/method consistency Change the code-folding button text from "Code" to "Show" (rstudio#2489) fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477) ...
This PR provides a fix for #2499 that doesn't address the root cause but will avoid the downstream errors caused by that issue.
In short, under unknown circumstances, the shinyrmd context script tags for dependencies and execution dependencies are written twice into the pre-rendered HTML files. rmarkdown then passes a length-2 character vector to
jsonlite::unserializeJSON()
, which causes a parsing error.This PR does two primary things:
I extracted the HTML and JSON parsing of dependencies into a single function
shiny_prerendered_extract_context_serialized()
. This function ensures a single character string is passed tojsonlite::unserializedJSON()
, using the last context in the document if more than one non-unique context is found.rmarkdown reads both dependency types in
shiny_prerendered_prerender()
(the function that decides if a pre-render run is required). At this point, we don't need to error if parsing fails and can instead trigger a pre-rendered pass to happen again. Ideally, this new pre-render pass will fix any issues in the pre-rendered HTML file, but if it somehow writes invalid dependency JSON again,shiny_prerendered_html()
will still fail (rather than infinitely looping).Note that avoiding an infinite loop is hypothetical; the current bug wouldn't trigger a new pre-render pass.