Add binding and el fields to shiny:inputchanged event #1596
Conversation
|
I'd prefer to walk through this PR together. |
wch
force-pushed
the
wch/setinput-binding
branch
from
6dc638c
to
a9f801b
Compare
| @@ -128,7 +136,13 @@ function initShiny() { | |||
|
|
|||
| var type = binding.getType(el); | |||
| var effectiveId = type ? id + ":" + type : id; | |||
| currentValues[effectiveId] = binding.getValue(el); | |||
| inputItems[effectiveId] = { | |||
There was a problem hiding this comment.
From discussion, it seems like the behavior we have today would cause immediate=FALSE when the exported bindAll is called. Please follow up to see if indeed immediate=FALSE. In any case we want this to be immediate going forward.
There was a problem hiding this comment.
PR #1595 (commit 0ef15fa) removed the following code in bindInputs because we thought it was redundant:
if (shinyapp.isConnected()) {
valueChangeCallback(binding, el, false);
}It turns out that the false in this call is important, and wasn't being used in the other call to inputs.setInput from exports.bindAll. So we'll need to need to make sure that the value of immediate is set properly here.
srcjs/input_rate.js
Outdated
| var InputEventDecorator = function(target) { | ||
| this.target = target; | ||
| }; | ||
| (function() { | ||
| this.setInput = function(name, value, immediate) { | ||
| this.setInput = function(name, value, opts) { | ||
| opts = Object.assign({ |
There was a problem hiding this comment.
It might be better to have a global default options object instead of creating it each time.
| var InputRateDecorator = function(target) { | ||
| this.target = target; | ||
| this.inputRatePolicies = {}; | ||
| }; | ||
| (function() { | ||
| this.setInput = function(name, value, immediate) { | ||
| this.setInput = function(name, value, opts) { |
There was a problem hiding this comment.
Instead of checking opts at each level of input decorator, it would be cleaner to add another level, like inputValidatorDecorator to the top of the stack, which checks that name isn't null, the opts are valid, and so on.
srcjs/shinyapp.js
Outdated
| @@ -29,7 +32,10 @@ var ShinyApp = function() { | |||
|
|
|||
| $.extend(initialInput, { | |||
| // IE8 and IE9 have some limitations with data URIs | |||
| ".clientdata_allowDataUriScheme": typeof WebSocket !== 'undefined' | |||
| ".clientdata_allowDataUriScheme": { | |||
There was a problem hiding this comment.
This can be moved init_shiny.
| @@ -4,6 +4,9 @@ var ShinyApp = function() { | |||
| // Cached input values | |||
| this.$inputValues = {}; | |||
|
|
|||
| // Input values at initialization (and reconnect) | |||
| this.$initialInput = {}; | |||
There was a problem hiding this comment.
this.$initialInput and this$inputValues should not keep opts, because it would prevent the el from being garbage collected. The opts are only used in the input decorator stack; there's no need to keep them outside of that.
This reverts commit 12c0b6e.
The new placement properly reflects the decorator stack
srcjs/init_shiny.js
Outdated
|
|
||
| var inputs = inputsValidate; |
There was a problem hiding this comment.
Total nitpick but I would've had var inputs above the if/else block, and have inputs = inputsRate and inputs = inputsDefer as before, and then after the if/else block, inputs = new InputValidateDecorator(inputs).
There was a problem hiding this comment.
That makes sense. I can make that change.
With this PR, the
shiny:inputchangedJavaScript event gets two new fields,bindingandel, which contain the input binding and DOM element, respectively. Additionally,Shiny.onInputChange()now accepts an optional argument,opts, which can contain the same fields.