search

daattali / shinyjs

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

Ensure that extendShinyjs does not overwrite other shinyjs functions #230

Closed DavidBarke closed 3 years ago

DavidBarke commented 3 years ago

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 commented 3 years ago

Good idea!

There is already a place that defines a list of some shinyjs functions https://github.com/daattali/shinyjs/blob/2404d5f714c6cc9531136da28a6d612082c53584/R/useShinyjs.R#L67

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.