rstudio / rmarkdown

Dynamic Documents for R
https://rmarkdown.rstudio.com
GNU General Public License v3.0
2.88k stars 975 forks source link

Using url in CSS argument throws an error now #2163

Closed cderv closed 3 years ago

cderv commented 3 years ago

This is a regression we have

Reproducible example :

dir.create(tmp_dir <- tempfile())
owd <- setwd(tmp_dir)
xfun::write_utf8(c(
  "---", 
  "title: test",
  "---", 
  "",
  "# test"), "test.Rmd")

rmarkdown::render("test.Rmd", 
                  rmarkdown::html_document(
                    css = "http://netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.css"
                  )
)

setwd(owd)
unlink(tmp_dir, TRUE)
Could not fetch http:/netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.css
InvalidUrlException "http:/netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.css" "URL must be absolute"
Error: pandoc document conversion failed with error 61
Run `rlang::last_error()` to see where the error occurred.

It must come from the new sass support in css arg I believe.

This was first reported in https://github.com/yihui/knitr/issues/1864#issuecomment-855301382

cderv commented 3 years ago

A quick git bisect shows that it happens in 98ba0e206b5208225ff1cfe6c386da375f955bbb (#2095)

cderv commented 3 years ago

Workaround for those stuck by this in rmarkdown 2.8 : pass the CSS arg asis to Pandoc

output:
    html_document:
        pandoc_args: ["--css", "http://netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.css"]

@vnijs @yihui this could solve the issue in https://github.com/yihui/knitr/issues/1864#issuecomment-855301382 without waiting for a new CRAN release.

Sorry for the trouble.

yihui commented 3 years ago

No worries! Thanks for the investigation and fix! Sounds like we'd better make a CRAN release relatively soon.

cderv commented 3 years ago

Yes sounds like it as this is a regression. This is the type of issue that makes it hard to plan releases 😅

However, I am surprised there was no issue open about this earlier. Using url in css argument does not seem to be used that often - I would have thought otherwise. So maybe this is not so urgent.

yihui commented 3 years ago

Without our CRAN release, it would hold @vnijs's release of radiant. It's been a month since the last release. If we were to use my previous release schedule, it's time to consider a release now, even if nothing is urgent.

cderv commented 3 years ago

Without our CRAN release, it would hold @vnijs's release of radiant

That was why I mentioned the workaround above https://github.com/rstudio/rmarkdown/issues/2163#issuecomment-855808083 that would work on radiant's vignette I think. Aim is to not block radiant.

But for sure we can do another release - no problem with that. as you said it is time and we have important fix in the current 2.9 version that would be good to get on CRAN 👍

I let you handle that and ping me if I can help with anything. Thanks !

yihui commented 3 years ago

@vnijs Would you like to use the workaround or prefer us updating rmarkdown on CRAN?

@cderv I just looked at NEWS.md, and personally I'm particularly excited about the fact that HTML widgets could use href components instead of being limited to file. IMO, that fix could justify a release. Usually I tend to make releases on a monthly basis if there are substantial bug fixes, so that no more users will be bitten by the same bugs that have already been fixed. This CSS bug is relatively new, and probably not many people would run into it, I'm still a little bit concerned if there really is only one package affected on CRAN. It would be too late and scary if I get a CRAN email informing me that the previous version broken other packages.

vnijs commented 3 years ago

Thanks @cderv. Just got confirmation that radiant is on its way to CRAN with the workaround!

cderv commented 3 years ago

I just looked at NEWS.md, and personally I'm particularly excited about the fact that HTML widgets could use href components instead of being limited to file. IMO, that fix could justify a release.

Exactly, that it is what I had in mind by "we have important fix in the current 2.9 version that would be good to get on CRAN" 😄 This one is important.

Just got confirmation that radiant is on its way to CRAN with the workaround!

@vnijs glad it help you in the short term. We'll get back to you when the fix is on CRAN so that you can change back to a more traditional YAML syntax.

pwildenhain commented 3 years ago

Just to chime in, we noticed this at our company where we take advantage of using a url for the css arg in the yaml header, to get our company themes/colors in all of our Rmarkdown reporting.

A release would be greatly appreciated 🙇🏻 👍🏻

cderv commented 3 years ago

Hi @pwildenhain !

You're right in time! We already pushed to CRAN (today!) the release with this fix : https://github.com/rstudio/rmarkdown/releases/tag/v2.9

On CRAN already: https://cran.r-project.org/web/packages/rmarkdown/index.html

Should be available as binaries in a few days.

github-actions[bot] commented 2 years ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.