Move shiny to Imports

[pawelru]

Depends type of dependency is generally discouraged. Would it be possible to move this to Imports instead?

[donyunardi]

With the current setup, we make sure that the shiny package is attached when teal.slice is attached.
Moving this doImports means that shiny will be installed but won't be attached when teal.slice is attached.

I look around the code and it looks like we always use shiny:: prefix when using shiny's function, but probably would need to be looked thoroughly.

[pawelru]

For exactly this reasons we already have shiny (among others) in Depeds of teal and I see the value in there. However here, in teal.slice, I am not so sure. It's somehow redundant given what teal has. Do we see any real use case of teal.slice alone (without teal)?

[chlebowa]

Do we see any real use case of teal.slice alone (without teal)?

I thought the intent was to allow it.

[pawelru]

Right. That was an inaccurate question.
Let me share my view on this so that you can better understand my point. I personally see packages with Depends as a meta packages that tells you "load me and I will load extra pkgs that you need". teal perfectly fits into this definition as we keep telling "please just do library(teal) and that it" (or to be more precise: library(teal.modules.xyz)). It's sort of a library(tidyverse) (even though I'm not using it).
I cannot find this true for teal.slice. I see it more as a rather low-level functionalities package which load should be accompanied with additional library(shiny) call - exactly how it is in the current "Getting started" vignette.
The other thing, as I mentioned, is that Depends is discouraged:

Unless there is a good reason otherwise, you should always list packages in Imports not Depends. That’s because a good package is self-contained, and minimises changes to the global landscape, including the search path.

source

[m7pr]

@pawelru we will need to add library(shiny) or use shiny:: as prefix in examples if we move shiny from Depends to Imports. A thing to remember when doing the transition.

[chlebowa]

No we won't, @import shiny will cover that.

[m7pr]

A test package https://github.com/m7pr/import_test
That only has @import shiny https://github.com/m7pr/import_test/blob/main/R/hello.R#L10
and shiny in Imports https://github.com/m7pr/import_test/blob/main/DESCRIPTION#L15
but fails on examples execution without library(shiny) nor prefixing

https://github.com/m7pr/import_test/actions/runs/7639811026/job/20813626827#step:40:107

❯ checking examples ... ERROR
  Running examples in 'r.pkg.template-Ex.R' failed
  The error most likely occurred in:
  
  > base::assign(".ptime", proc.time(), pos = "CheckExEnv")
  > ### Name: hello
  > ### Title: Personal greeting
  > ### Aliases: hello
  > 
  > ### ** Examples
  > 
  >  isolate()
  Error in isolate() : could not find function "isolate"
  Execution halted

1 error ✖ | 0 warnings ✔ | 0 notes ✔
Error: R CMD check found ERRORs
Execution halted

Exited with status 1.

[chlebowa]

It is because they are not re-exported.

I stand corrected 👍

[m7pr]

I'm not sure what's going on. @import shiny has nothing to do with examples execution and has no impact on them, so what was your point in here?

[m7pr]

For now we have prefixing in examples and library(shiny) on main, so examples and vignettes will work no matter shiny is in Depends or Imports in DESCRIPTION file and I think NAMESPACE, hence @import shiny has no impact on shiny functions being used in examples. It has effect on shiny functions being used in teal.slice functions.

However this active and soon to be closed PR #506 removes prefixing and library(shiny) calls, so once this PR is merged and then shiny is moved from Depends to Imports in DESCRIPTION file, examples will not work.

[m7pr]

So we either

• do need to have shiny:: prefix and library(shiny) calls in examples if we have shiny in Imports (proposed in this PR)
• do not need to have shiny:: prefix and library(shiny) calls in examples if we have shiny in Depends (current main)

[donyunardi]

There are too many docs that we need to double check if we move this to Imports and we're too close for release.
Let's explore this after the release.

[averissimo]

• do need to have shiny:: prefix and library(shiny) calls in examples if we have shiny in Imports (proposed in this PR)

It might not be that big change in the option that @m7pr mentioned, granted it's 17 files that need to be touched, but there's only 1 user-facing example (FilterPanelAPI, all others are internal ones)

devtools::test(), devtools::runExamples(fresh = TRUE) and devtools::check() run fine with adding library(shiny) when needed

[m7pr]

Yeah, we have GHA tests/flows that can verify if that transition (from Depends to Imports) and change in examples gives the proper build of the package. I assume we can just see if there is anything requiring shiny in the example of the function, and then add library(shiny) and basically that's the whole change. And you go through all examples.

[averissimo]

these change seem to do the trick. Pushed a sandbox branch (based on the current pre-release) that can serve as the reference point when implementing this

[pawelru]

Please also don't forget about vignettes (and possibly README.md)

[averissimo]

The check function rebuilds the vignettes and they already use the library(shiny) on the relevant chunks.

Good point about the README.md, but it's super simple without any working examples (it points to the vignettes)