tagQuery(): Relocate html deps to child objects

[schloerke]

Fixes #301
cc @DavidJesse21

• test added
• news entry
• document that tag query will relocate all htmldeps after usage (and also flatten nested lists in children)

[wch]

I'm not saying we need to do this now, but there's another possible way to approach this which may simplify things in the long run: in the tag to tag-environment conversion, convert all html dependency attributes into children. This would provide a normalized data structure where the html deps are always children, instead of sometimes being children and sometimes being attributes.

This does mean that when the tag env object is converted back into a tag, the structure will be changed, but I think that already happens in other ways.

If we want to push this even further, it may make sense to always make html deps children, even on regular tag objects. For example, attachDependencies() would simply add the dependencies as children, instead of as attributes. I haven't thought about this enough yet, though, to say whether or not there would be unexpected breakage from doing this. If we could always use a normalized format, it would simplify our code in the long run; the only issue is if whether it would cause problems in the short term.

[cpsievert]

If we want to push this even further, it may make sense to always make html deps children, even on regular tag objects. For example, attachDependencies() would simply add the dependencies as children, instead of as attributes.

FWIW, I vaguely recall going down this road for something else and I'm pretty sure I backed out because it was going to break a lot of stuff (maybe because lots of people were using attr(x, "html_dependencies") directly?)

[schloerke]

While I like the list item structure as well, I agree with @cpsievert . Moving to a list item completely breaks retrieving html deps using htmlDependencies(x)

---

If we could always use a normalized format, it would simplify our code in the long run; the only issue is if whether it would cause problems in the short term.

I'm totally up for standardization, but it will cause breakages in the near term.

[wch]

I think it's arguable that the current behavior of htmlDependencies() is incorrect, when it comes to dealing with deps that are added as children:

library(htmltools)
x <- div("hello", htmlDependency("foo", "1.0", "/foo"))
htmlDependencies(x)
#> NULL

findDependencies() knows how to handle them, though:

findDependencies(x)
#> [[1]]
#> List of 10
#>  $ name      : chr "foo"
#>  $ version   : chr "1.0"
#>  $ src       :List of 1
#>   ..$ file: chr "/foo"
#>  $ meta      : NULL
#>  $ script    : NULL
#>  $ stylesheet: NULL
#>  $ head      : NULL
#>  $ attachment: NULL
#>  $ package   : NULL
#>  $ all_files : logi TRUE
#>  - attr(*, "class")= chr "html_dependency"

In this search I see some cases where the code accesses attr(x, "html_dependencies"), but not a prohibitively amount:
https://github.com/search?q=org%3Acran+html_dependencies&type=code

At any rate, this isn't something we necessarily need to do in this PR, but if not, we should open another issue about it.

[wch]

We can merge this, but should also file an issue referring back to the discussion here, about normalizing the html dependency data structures in general.

[DavidJesse21]

Hey, first of all thank you all for catching up on this issue!
I have now downloaded the dev version of {htmltools} via:

remotes::install_github("rstudio/htmltools", ref = "taglist_squash")

Without looking into details I cannot notice any differences regarding my example given here.
Have I just done something wrong downloading the latest version with the fixes or how can you explain that my problem preserves?

Cheers
David

[schloerke]

Dang. Still missed some. 😞 Will explore more soon.
@DavidJesse21 Your code is great. You are doing nothing wrong.

---

Maybe we use @wch's approach on the HTML deps for anything that goes through tagQuery(). Keeping track of all the dep attributes attributes when subsetting ([[), unlist()ing, and flattening children is becoming overly difficult.

[schloerke]

@DavidJesse21 Can you check again? I have confirmed it on my side, but want to be sure.

---

Changes:

• When sending a set of tags to tagQuery():
  • All tag html deps (in attrs) will be appended to the tag's $children field.
  • All tag lists will remove all html dependencies (in attrs) and append them to the end of the list

This works recursively and will expand on the children slots and remove the brittleness of attributes when flattening all $children fields.

This change seems warranted as the $children structure was not guaranteed to be the same after sending to tagQuery(). Now, this also extends to the location of the html dependencies.

The agreement of tagQuery() still stands that when rendering the html tags both the input tags and output tags (of tagQuery()) should render the same result. (The internal structures will probably change.)

[DavidJesse21]

This seems to work now, thank you :)