search

daattali / shinyjs

💡 Easily improve the user experience of your Shiny apps in seconds
https://deanattali.com/shinyjs
Other
734 stars 119 forks source link

extendShinyjs: missing documentation of special id treatment #229

Closed DavidBarke closed 3 years ago

DavidBarke commented 3 years ago

Hello Dean,

if I call a extendShinyjs function from R with argument id, it gets pasted with the namespace:

# R side
js$fun(id = "id")
// JS side
shinyjs.fun = function(params) {
  params.id // is ns(id)
};

I could pass asis = TRUE to prevent this.

This might be either a bug due to unexpected behavior or should be documented better. The documentation for extendShinyjs already includes an example with argument id:

https://github.com/daattali/shinyjs/blob/2404d5f714c6cc9531136da28a6d612082c53584/R/extendShinyjs.R#L94-L98

Maybe that would be a appropriate location to include this information?

Best reagrds David

daattali commented 3 years ago

Thanks for bringing to my attention. I wanted the id parameter in shinyjs functions to automatically get namespaced so that it would be in-line with how shiny works - in shiny whenever you do anything in the server, you don't need to specify a namespace with an ID, it's inferred. However, I was only thinking about the built in shinyjs functions when I did that. It makes sense for those functions because I control what those functions are and I know that id for those functions always refers to an elemtn ID from the UI and that the namespace would be correct.

However, I didn't have in mind custom extendShinyjs functions, and the fact that they can also have an id parameter. I'm actually not entirely sure what should be the behaviour in that case. Custom functions can operate on anything, and id doesn't necessarily correspond to a namespaced ID of an element from the shiny UI. I'm not sure if this behaviour should remain (and be documented), or if it should be reverted.

DavidBarke commented 3 years ago

Not namespacing id would probably be what most users expect. However, given the fact that you added extendShinyjs more than five years ago, I can't imagine to be the first one trying to call a js function from within a module with argument id. Maybe others just noticed this issue without reporting it.

Reverting this behaviour shouldn't be too difficult when #230 is merged, as one would just need to test in jsFuncHelper whether this function name is in the shinyjs function names or not.

daattali commented 3 years ago

I'm generally very opposed to modifying behaviour in a breaking way, but I don't think many people use id in custom js functions inside modules. I think it'd be a good idea to remove automatic namespacing from all custom functions.