add shiny_prerendered special comment via pandoc_include_args #2249

atusy · 2021-11-24T15:26:54Z

instead of pandoc_variable_args and templates.

In this way, we can add  even for custom templates, for sure.
If this is unlikely, I think we should document the need of this comment somewhere else not only in NEWS.md.

atusy · 2021-11-24T15:31:54Z

Sorry, I worked on a wrong commit tree. I made fix and forced push.

yihui

That looks like a good idea to me. Thanks!

@cderv You can have another look.

cderv · 2021-11-24T17:17:51Z

Thanks, I'll have a look. We need to test this against learn basically (cc @gadenbuie ) and I need to check why we did it that way. I remember something with header-includes possibly not used or that the order matters.

Anyway, if we can do the change safely then that would be great to support custom template without having developers to know about this.

gadenbuie · 2021-11-24T19:13:05Z

@atusy this is a very nice solution to use the widely used $header-includes$ pandoc template variable to ensure that custom templates don't need to add special treatment for shinyrmd (or shiny_prerendered) support. Thank you!

We added this originally because dynamic HTML dependencies in shiny Rmds were appearing higher up in the <head> element before other things, like jQuery and other dependencies. I think that as long as we ensure that the shiny-header.html is added to the end of the header_includes everything will work as expected. I tested this locally and it appears to work as expected with learnr.

We should also consider what happens if a user uses a template that already contains . The current behavior is to use the first of these that is found. So this change won't impact templates where a manually added  appears before $header-includes$ , just those where it appears after. I think the impact is likely to be small and the new behavior will probably be equally correct, but it's worth noting the change.

atusy · 2021-11-25T12:18:35Z

The current behavior is to use the first of these that is found. So this change won't impact templates where a manually added  appears before $header-includes$, just those where it appears after.

Oh yes, this is the good point, thank you @gadenbuie . I agree it is worth noting it.

cderv

THanks @atusy.

I needed time to go back in time regarding the change I made initially and to fully grasp what this change means.

If I may go back to the why, what issue are we trying to solve here exactly ?

Change made in version 2.8 with #2064 was to bring a flexible mechanism.

Previous and default behavior was for shiny prerendered to include the HTML dependency content at the end of the <head>.
However, this was not enough for some template and we allowed to add the special comment anywhere in a custom template by using the variable set when shiny prerendered happens. This solved several issues in learnr because of how dependencies where loaded - some stuff in the head depending

With this PR, we are enforcing the later. This means that in all case shiny prerendered will insert its stuff at the same place as header-includes in the template.

Am I understanding correctly ? Do we agree on that ?
It seems the note made in NEWS is about that

If templates rely on the old behavior and require some contents between $header-includes$ and 

but there is not always a , sometimes just the end of the <head>.

I think it is fine doing that as there are not a lot template using this feature, but I want to be sure. I know I spent time on the previous change so I want to check that we are doing things correctly and need to look into this a bit more.

cderv

I guess my interrogation is do we still need the previous fix from #2064 that aimed to be an improvement over #1942, which ended up by doing

rmarkdown/R/shiny_prerendered.R

Lines 179 to 192 in 880923d

    
           # The html template used to render the UI should contain the placeholder 
        
           # expected by shiny in `shiny:::renderPage()` which uses 
        
           # `htmltools::renderDocument`. 
        
           # If it is not present in the template, we add this placeholder at the end of 
        
           # the <head> element 
        
           if (!any(grepl(headContent <- "<!-- HEAD_CONTENT -->", html_with_deps, fixed = TRUE))) { 
        
             html_with_deps <- sub( 
        
               '</head>', 
        
               paste0('\n', headContent, '\n</head>'), 
        
               html_with_deps, 
        
               fixed = TRUE, 
        
               useBytes = TRUE) 
        
             Encoding(html_with_deps) <- "UTF-8" 
        
           }

With the solution of this PR, the special comment will always be added into the template (unless this is a template without $header-includes$ but I guess it is rare and probably won't work on other stuff).

Can we say that this PR offers a more generic solution to the initial problem of #1912 that the two previous PR tried to solve ?

I keep going back to the issue we are trying to solve because that is the most important: why do a change.

it seems like we can use your solution @atusy by always inserting the comment in header-includes. This means to always add the html deps by shiny at the same place as we regular documents. Seems quite right to me do that.

Does my thinking make sense ?

gadenbuie · 2022-02-09T15:25:45Z

Can we say that this PR offers a more generic solution to the initial problem of #1912 that the two previous PR tried to solve ?

I agree with this. I think @atusy's approach here is quite elegant and addresses the issue that html dependencies would be loaded in different places when an Rmd is rendered as a static document vs when rendered as a shiny document. This PR would solve the problem for (almost) all pandoc templates.

With this PR, we'd now have three options:

Do nothing and runtime: shiny will add  in the right place.
Manually add  to the pandoc template, as long as it's above $header-includes$ . This option would only be taken by the most diligent devs who follow rmarkdown very carefully. In other words, it's safe to assume very few people are using this feature.
If all else fails, for some reason, then we put the html dependencies just before the closing </head>. Ideally this happens almost never, but it's reasonable to keep around just in case.

In summary, the justification for this PR is that it means that we're ensuring consistent behavior with and without runtime: [shiny | shinyrmd | shiny_prerendered] for all pandoc templates.

cderv · 2022-02-09T15:42:44Z

Thanks - I am glad that confirms my understanding.

So @gadenbuie you would keep the current change from #2064 so the your case 3 works ? Only case I see this is for a document with no $header-includes$ - I think this case will never happens with rmarkdown and we would have some code just for this. I don't know if that is good in the long term - but I can document properly the code.

I guess 1 and 2 are enough, and mostly 1 will cover all case correctly. 2 will work as long as htmltools uses a sub() and not a gsub() 😅

Anyway, I am now convinced that this PR is a better solution for the initial issue. Thank you @atusy !

atusy · 2022-02-11T14:27:51Z

Thank you @cderv for your neat discussion and @gadenbuie for your clear aswer!

I remember I dealt with this issue when I apply KaTeX in atusy/minidown.
However, I do not remember the detail and the reason why my template does not contain the ...
Maybe I guessed this PR be merged promptly because of yihui's quick approval.
I should have kept the note by opening an issue in my repo...

cderv · 2022-02-11T14:42:09Z

No worries, Opening issues and discussing in PR are great way to document things. We often remember that when we do not do it. 😅

I'll merge this now as it ends up being the right solution IMO - I was waiting for your last feedback.

I remember I dealt with this issue when I apply KaTeX in atusy/minidown.

rmarkdown now has support for several math engine using math_method argument - but you know that ! 😄

atusy · 2022-02-11T14:50:26Z

rmarkdown now has support for several math engine using math_method argument - but you know that !

Of course, and supporting it is my milestone for minidown 0.5.0!

cderv · 2022-02-11T14:53:38Z

Of course, and supporting it is my milestone for minidown 0.5.0!

#2304 could be even better for minidown maybe, to keep the HTML output without JS deps. Anyway ! Happy to help somehow if I missed something for easy integration in other format, but I think you have nothing really to do (unless you did some special treatment for Math already). Not the place to discuss though, I'll stop :)

atusy · 2022-02-11T14:55:12Z

Thanks again!

Linking to rstudio#2249

[skip ci]

add comment by pandoc_include_arg instead of pandoc_variable_arg

b875f1a

atusy force-pushed the shinyrmd branch from 0dbcb13 to b875f1a Compare November 24, 2021 15:29

Merge remote-tracking branch 'upstream/main' into shinyrmd

21161f9

atusy added 2 commits November 25, 2021 00:45

update test

089d557

update NEWS

2eeed16

yihui approved these changes Nov 24, 2021

View reviewed changes

yihui requested a review from cderv November 24, 2021 16:21

gadenbuie mentioned this pull request Nov 24, 2021

Update tutorial pandoc template for head content in header-includes rstudio/learnr#613

Merged

atusy and others added 2 commits November 25, 2021 21:31

update NEWS

84150cd

Merge branch 'main' into shinyrmd

de5855f

cderv reviewed Feb 9, 2022

View reviewed changes

cderv added 6 commits February 11, 2022 16:02

Add comment about why we are doing this

df1e894

Linking to rstudio#2249

Update NEWS

ab74186

be more precise on test

2fd4a17

use grep directly instead of which(grepl(

b9e1d4a

Add a test to check it inserts in header-includes directly.

ee77394

bump version

17d3cb3

[skip ci]

cderv changed the title ~~when is_shiny_prerendered(), add HEAD_CONTENT comment via pandoc_include_args~~ add shiny_prerendered special comment via pandoc_include_args Feb 11, 2022

cderv merged commit 4c7b462 into rstudio:main Feb 11, 2022

github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add shiny_prerendered special comment via pandoc_include_args #2249

add shiny_prerendered special comment via pandoc_include_args #2249

atusy commented Nov 24, 2021

atusy commented Nov 24, 2021

yihui left a comment

cderv commented Nov 24, 2021

gadenbuie commented Nov 24, 2021

atusy commented Nov 25, 2021

cderv left a comment

cderv left a comment •

edited

Loading

gadenbuie commented Feb 9, 2022

cderv commented Feb 9, 2022

atusy commented Feb 11, 2022

cderv commented Feb 11, 2022

atusy commented Feb 11, 2022

cderv commented Feb 11, 2022

atusy commented Feb 11, 2022

	# The html template used to render the UI should contain the placeholder
	# expected by shiny in `shiny:::renderPage()` which uses
	# `htmltools::renderDocument`.
	# If it is not present in the template, we add this placeholder at the end of
	# the <head> element
	if (!any(grepl(headContent <- "<!-- HEAD_CONTENT -->", html_with_deps, fixed = TRUE))) {
	html_with_deps <- sub(
	'</head>',
	paste0('\n', headContent, '\n</head>'),
	html_with_deps,
	fixed = TRUE,
	useBytes = TRUE)
	Encoding(html_with_deps) <- "UTF-8"
	}

add shiny_prerendered special comment via pandoc_include_args #2249

add shiny_prerendered special comment via pandoc_include_args #2249

Conversation

atusy commented Nov 24, 2021

atusy commented Nov 24, 2021

yihui left a comment

Choose a reason for hiding this comment

cderv commented Nov 24, 2021

gadenbuie commented Nov 24, 2021

atusy commented Nov 25, 2021

cderv left a comment

Choose a reason for hiding this comment

cderv left a comment • edited Loading

Choose a reason for hiding this comment

gadenbuie commented Feb 9, 2022

cderv commented Feb 9, 2022

atusy commented Feb 11, 2022

cderv commented Feb 11, 2022

atusy commented Feb 11, 2022

cderv commented Feb 11, 2022

atusy commented Feb 11, 2022

cderv left a comment •

edited

Loading