Ensure that extendShinyjs does not overwrite other shinyjs functions

[DavidBarke]

I noticed that one could overwrite other shinyjs functions using extendShinyjs, which is probably not intended. I therefore added an additional check to extendShinyjs.

[daattali]

Good idea!

There is already a place that defines a list of some shinyjs functions

shinyjs/R/useShinyjs.R

Line 67 in 2404d5f

jsFuncs <- c("show", "hide", "toggle", "enable", "disable", "toggleState",

I'd be worried about introducing another similar list, it would be very easy to forget to update it.

I think the better approach here would be to define these lists elsewhere , perhaps in the .globals variable. Your new list would be a concatenation of my list plus some other functions. That way there's only one place where these functions are defined. Does that make sense?

[DavidBarke]

Thanks for your feedback. That makes sense. I added a helper function shinyjsFunctionNames that returns the appropriate names based on the type argument.

I hope you are okay with a helper function instead of using the .globals variable.

[daattali]

The helper function is good. A few things to fix:

1. It seems the current code breaks when any of the functions are used, the shinyjsFunctions should probably be shinyjsFunctionNames("all")
2. init should be allowed. It's actually meant to be written by the user
3. Instead of listing ALL the illegal funciton names, I would do say which ones are the offending ones in the error message

[DavidBarke]

I fixed the mentioned issues. I removed "init" from shinyjsFunctionNames. Therefore type ="all" might be confusing, when all function names except for "init" are returned. I could add another type "extendShinyjs", but this might be overengineered for the current use case.

By the way, how do you test any changes you make to your package?

[daattali]

Unfortunately I don't have any good way of automatic testing for this package. That's one big disadvantage of writing sihny packages rather than regular R packages. If you're up for it I would love to strat accumulating some tests for shinyjs.

[DavidBarke]

I imagine this to be quite a huge task, if one would do this for every aspect of your package. There is probably no single solution that covers all aspects. I guess you could use the shinytest package or the new shiny::testServer, but even then you still need a test tool like Selenium to test things that happen on the JS side.

What kind of tests do you consider meaningful? I noticed you already have tests for hidden. You probably added them as it was easy to test them with testthat?

[daattali]

This package was created before any testing frameworks for shiny existed, so the only thing I could test at the time was pure R code. You're right this would be a big undertaking, but even if just some of the core functions (show/hide/disabled/enabled/reset) are tested that would already be a big improvement. I'm open to suggestions for what would be the best apprroach, I haven't given it too much thought, I've been focusing on other packages recently.