Depricate and remove show()

[ralmond]

I'm having numerous problems because shinyjs overrides methods::show, which is called from places deep in the system, and hence hard to override.

In particular, I have a reference class object which I've written $show() methods for. However, when I try to print a list of these objects, it causes problems.
For example:

Browse[2]> parents[TRUE]

$PropertiesofTorque

Error: shinyjs: could not find the Shiny session object. This usually happens when a shinyjs function is called from a context that wasn't set up by a Shiny session.

Somehow, the call to show is happening within the method for show.
I'm similarly getting problems in R shiny as the object produce errors when the shiny display trys to display them.

I'm not sure there is any fix for this problem other than removing "show" so that it now longer masks methods::show.

[daattali]

Can you share a minimal code example that has this issue?

I've added showElement()/hideElement() a long time ago especially because of this. But I never ran into a case where it was unavoidable to call shinyjs::show instead of methods::show, unless the underlying package has old code that doesn't namespace the show function

I would be extremely hesitant to deprecate show/hide because they are very widely used functions and it would cause a lot of chaos for many people

[ralmond]

I'll work on a minimal example. I think it may be related to the way the (internal) print function treats references classes inside of lists. It might take me a day or so. (The maximal example involves the NeticaBN and NeticaNode classes in my package RNetica, which is not on CRAN, but is available from https://pluto.coe.fsu.edu/RNetica. Printing lists of those objects generates the error shown above.")

I fully understand your reluctance to remove 'show()'. I almost hesitated to ask, as I strongly suspect it would break a lot of code. I think I was more suggesting that if you formally depreciate it now and and encourage people to move to 'showElement()/hideElement()', it would allow you to remove it in the future.

[daattali]

Usually when I saw this problem happen, it was because the underlying code made a non-namespaced call to 'show()', and the fix (which should really have been done regardless of this issue) is to change it to 'methods::show()'.

So I'm curious to see if you found an instance where this issue presents even in correct code.

Thanks for understanding

On Fri, Nov 1, 2019, 14:31 ralmond <notifications@github.com wrote:

I'll work on a minimal example. I think it may be related to the way the (internal) print function treats references classes inside of lists. It might take me a day or so. (The maximal example involves the NeticaBN and NeticaNode classes in my package RNetica, which is not on CRAN, but is available from https://pluto.coe.fsu.edu/RNetica. Printing lists of those objects generates the error shown above.")

I fully understand your reluctance to remove show()'. I almost hesitated to ask, as I strongly suspect it would break a lot of code. I think I was more suggesting that if you formally depreciate it now and and encourage people to move to showElement()/hideElement()`, it would allow you to remove it in the future.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/daattali/shinyjs/issues/195?email_source=notifications&email_token=AAHIQFA2XXFUZW5EWJ7VDOLQRSN4FA5CNFSM4JHAVKR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC4G6PQ#issuecomment-548958014, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHIQFF6GU6HQ3LZSQLG3FTQRSN4FANCNFSM4JHAVKRQ .

[daattali]

@ralmond please let me know if you still find this issue

[ralmond]

My apologies for not getting back sooner.
I created what I hoped would be a small test case (the minimal setup involves setting up a small package with a reference class which has a $show method, but not a method for the methods::show() generic function. Unfortunately, the bug didn't reproduce in a small test case. At that point, I got stumped trying to figure out how to post an entire package to the message board.

Fortunately, I do think I have found a work-around.
Instead of adding shinyjs to the Depends: header in the DESCRIPTION file, I add it to imports:. I then specifically import the shinyjs files I'm using in the NAMESPACE file.

importFrom(shinyjs, useShinyjs, toggleState)

This should not override methods::show() and hence shouldn't interfere with the printing system of R or RStudio.

For the record, I think naming something the same as the commonly used methods package is likely to lead to problems of this sort. That is why I think you should formally recommend to users that they replace calls to shinyjs::show to calls to showElement() and eventually phase out shinyjs::show()

[daattali]

I agree that moving it from Depends to Imports is a good idea - I try to never have any packages in my Depends except for maybe shiny.

And I agree that naming these methods was not a good idea, it was done many years ago before I knew about the methods package, and when I didn't imagine anyone would use shinyjs. Unfortunately now I can't remove these functions because they are very widely used.