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

Add missing input binding method getType #26

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

alandipert
Copy link
Collaborator

The reactR-generated Shiny input binding was missing an implementation for getType, which is required in order for shiny::registerInputHandler to work.

This PR adds an implementation that simply identifies the input by the unique name we're already collecting from the user.

…. Fixes shiny::registerInputHandler with reactR inputs.
alandipert added a commit to react-R/colorpicker-example that referenced this pull request Apr 23, 2019
@alandipert alandipert changed the title Add missing getType Add missing input binding method getType Apr 23, 2019
@alandipert alandipert changed the title Add missing input binding method getType Add missing input binding method getType Apr 23, 2019
@timelyportfolio
Copy link
Collaborator

@alandipert makes sense. I will likely wait a couple of weeks to avoid upsetting our friends at CRAN.

@timelyportfolio timelyportfolio merged commit b8fd6a9 into master Apr 23, 2019
@timelyportfolio timelyportfolio deleted the input-impl-getType branch April 23, 2019 19:17
@timelyportfolio
Copy link
Collaborator

timelyportfolio commented Apr 27, 2019

@alandipert just got to play with this and my rfabric examples. I am seeing

Listening on http://127.0.0.1:6004
Warning: Error in <Anonymous>: No handler registered for type checkbox:rfabric.faCheckbox
  [No stack trace available]
Error in (function (name, val, shinysession)  : 
  No handler registered for type checkbox:rfabric.faCheckbox

Will package authors need to change their code to work with this change? Or will the scaffold need to be changed?

I believe we are missing a registerInputHandler() on the R side that we will need to add in the scaffold on .onLoad. Does adding getType to the JavaScript require a handler on the R side?

@alandipert
Copy link
Collaborator Author

Hm, I didn't expect them to. I probably misunderstood what getType() is responsible for exactly 🤦‍♂ . I'll be able to look into this next week, and look forward to hearing whatever you discover.

@timelyportfolio
Copy link
Collaborator

timelyportfolio commented Apr 27, 2019

@alandipert I mistakenly thought that getType() did not force require registerInputHandler(), but these Shiny lines seem to suggest that it does. In addition, the only registerInputHandler provided for official Shiny inputs also have a getType(). All the others don't specify getType so they use default getType which returns false.

As of now, I think we should revert this commit, but it does seem we need to find a way for a user to provide a getType() if necessary.

@timelyportfolio
Copy link
Collaborator

@alandipert these Shiny lines confirm that a handler is required if getType returns a : separated string.

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

Successfully merging this pull request may close these issues.

2 participants