Skip to content
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

resourcePath() to complement addResourcePath() #2401

Closed
ambevill opened this issue Apr 25, 2019 · 6 comments
Closed

resourcePath() to complement addResourcePath() #2401

ambevill opened this issue Apr 25, 2019 · 6 comments

Comments

@ambevill
Copy link

Would it be possible to add a getter function resourcePath() to return .globals$resources and .globals$resourcePaths? That way the code could test whether a particular resource has already been added.

@alandipert
Copy link
Contributor

alandipert commented Apr 25, 2019

Hm, interesting. Since we already have addResourcePath() I can see an argument for adding resourcePaths().

May I ask what you'd like to do that adding such a function makes easier/possible?

@ambevill
Copy link
Author

Sure---a lot of third-party packages automatically call addResourcePath() as a side-effect of some other important function. But in practice this is an unreliable way to make sure aRP is called.

For example, library(shinyBS) makes the appropriate aRP calls for shinyBS. But if my app uses shinyBS::xyz syntax, then aRP isn't called. This has caused issues for me, especially when packaging shiny apps. I've had this issue in shinyjs and shinyBS.

So the idea of resourcePaths() is that the third-party package developer could test whether aRP has been called and either issue a warning or make the call. And I could put checks in my code to make sure I haven't worked around the addResourcePath call somehow.

Maybe an alternative to shiny::resourcePaths() would be to specify a best-practice that the third-party package should use to call aRP. It looks like shinyBS may have the right idea. But then we should campaign a bit to make sure the third-party devs are implementing it this way.

@alandipert
Copy link
Contributor

alandipert commented Apr 25, 2019

Thank you very much for clarifying.

Have you considered placing your addResourcePath() calls in an .onLoad hook? My understanding is that this is the preferred way to perform package-level side effects at package load time.

I ran into a similar situation that demanded .onLoad recently, which was invoking shiny::registerInputHandler in a package that exports a custom Shiny input. Using .onLoad, my input seems to work for both the library(colorpicker) and colorpicker::colorpickerInput cases.

In any event, I think your original request might still be a good enhancement, so I'll consult with the team to determine if it's a change we would consider.

@ambevill
Copy link
Author

A .onLoad hook sounds like the right approach. I'll suggest this to the third-party devs when I encounter problems.

Maybe it would be good to suggest this in the addResourcePath and shiny::registerInputHandler documentation? (Arguably using .onLoad is the TPL developers' responsibility, but it can't hurt to suggest it.)

@alandipert alandipert added the Consult Team Requires consulting the Shiny team label Apr 25, 2019
@alandipert
Copy link
Contributor

alandipert commented Apr 25, 2019

Thank you, yes. I conferred with @wch and we both agree that mentioning .onLoad in the docs for addResourcePath and registerInputHandler is a good idea.

A PR for such a change would be welcome, but if you have better things to do, don't worry we'll get to it eventually 😃

Separately, we do think there's a decent but much less compelling argument for adding an a resourcePaths() accessor function. A good return value format would be a named vector, where the name is the URL and the value is the path on disk.

@alandipert alandipert added Docs Help Wanted and removed Consult Team Requires consulting the Shiny team labels Apr 25, 2019
@cpsievert
Copy link
Collaborator

Done in #2459 (there is now a resourcePaths() and removeResourcePaths())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants