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.

Sign up for GitHub

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

fix: Allow bindInputs() to no-op when attempting to bind currently bound inputs #3946

Merged
merged 22 commits into from
Nov 30, 2023
Merged

fix: Allow bindInputs() to no-op when attempting to bind currently bound inputs #3946

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
657be0a
fix: Do not re-bind previously bound inputs
gadenbuie Nov 29, 2023
3aa2f6c
refactor: Add binding to the registry after binding happens
gadenbuie Nov 29, 2023
0c03210
fix: Spelling of `bindingsRegistry`
gadenbuie Nov 29, 2023
7969054
chore: yarn build
gadenbuie Nov 29, 2023
554bac5
`yarn build` (GitHub Actions)
gadenbuie Nov 29, 2023
1b9ceab
fix: spelling
gadenbuie Nov 29, 2023
9d1f5c7
feat: isRegistered can check if bound to input or output
gadenbuie Nov 29, 2023
6eba4cf
fix: Do not throw for shared input/output IDs
gadenbuie Nov 29, 2023
f1a072e
fix: check element directly to know whether it a bound input
gadenbuie Nov 29, 2023
3e00632
chore: yarn build
gadenbuie Nov 29, 2023
26e5109
fix: test `.shiny-bound-input` instead of data prop
gadenbuie Nov 29, 2023
b67b062
refactor: Remove `bindingsRegistry.isRegistered()` method
gadenbuie Nov 29, 2023
616bda0
refactor: Use a map for duplicateIds again
gadenbuie Nov 29, 2023
b124d1b
refactor: Add `BindingTypes` type and use `bindingType` everywhere
gadenbuie Nov 29, 2023
1c0c4c3
Merged origin/main into fix/error-console-insert-ui
gadenbuie Nov 29, 2023
76abdca
refactor: More concise duplicateIds typing
gadenbuie Nov 30, 2023
4943f03
refactor: count by forEach + incrementing
gadenbuie Nov 30, 2023
4a8ca4b
`yarn build` (GitHub Actions)
gadenbuie Nov 30, 2023
75b50c6
thanks, vscode
gadenbuie Nov 30, 2023
6883344
docs: rewrite checkValidity() jsdoc to capture current state of things
gadenbuie Nov 30, 2023
5be1e65
chore: yarn build
gadenbuie Nov 30, 2023
a6b7c11
docs: slight rewording
gadenbuie Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 44 additions & 41 deletions inst/www/shared/shiny.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18553,28 +18553,31 @@
inputs.setInput(id, value, opts);
}
}
var bindingsRegistery = function() {
var bindingsRegistry = function() {
var bindings = /* @__PURE__ */ new Map();
function checkValidity() {
var duplicateIds = /* @__PURE__ */ new Map();
bindings.forEach(function(inputOrOutput, id) {
if (inputOrOutput.length > 1) {
duplicateIds.set(id, inputOrOutput);
var duplicateIds = {};
bindings.forEach(function(idTypes, id) {
var nInputs = idTypes.filter(function(s4) {
return s4 === "input";
}).length;
var nOutputs = idTypes.filter(function(s4) {
return s4 === "output";
}).length;
if (nInputs > 1 || nOutputs > 1) {
duplicateIds[id] = {
input: nInputs,
output: nOutputs
};
}
});
if (duplicateIds.size === 0)
var nDuplicates = Object.keys(duplicateIds).length;
if (nDuplicates === 0)
return {
status: "ok"
};
var duplicateIdMsg = Array.from(duplicateIds.entries()).map(function(_ref) {
var _ref2 = _slicedToArray2(_ref, 2), id = _ref2[0], idTypes = _ref2[1];
var counts = {
input: 0,
output: 0
};
idTypes.forEach(function(idType) {
counts[idType]++;
});
var duplicateIdMsg = Object.entries(duplicateIds).map(function(_ref) {
var _ref2 = _slicedToArray2(_ref, 2), id = _ref2[0], counts = _ref2[1];
var messages = [pluralize(counts.input, "input"), pluralize(counts.output, "output")].filter(function(msg) {
return msg !== "";
}).join(" and ");
Expand All @@ -18584,28 +18587,28 @@
status: "error",
error: new ShinyClientError({
headline: "Duplicate input/output IDs found",
message: "The following ".concat(duplicateIds.size === 1 ? "ID was" : "IDs were", " repeated:\n").concat(duplicateIdMsg)
message: "The following ".concat(nDuplicates === 1 ? "ID was" : "IDs were", " repeated:\n").concat(duplicateIdMsg)
})
};
}
function addBinding(id, inputOrOutput) {
function addBinding(id, type) {
if (id === "") {
throw new ShinyClientError({
headline: "Empty ".concat(inputOrOutput, " ID found"),
headline: "Empty ".concat(type, " ID found"),
message: "Binding IDs must not be empty."
});
}
var existingBinding = bindings.get(id);
if (existingBinding) {
existingBinding.push(inputOrOutput);
existingBinding.push(type);
} else {
bindings.set(id, [inputOrOutput]);
bindings.set(id, [type]);
}
}
function removeBinding(id, inputOrOutput) {
function removeBinding(id, type) {
var existingBinding = bindings.get(id);
if (existingBinding) {
var index = existingBinding.indexOf(inputOrOutput);
var index = existingBinding.indexOf(type);
if (index > -1) {
existingBinding.splice(index, 1);
}
Expand Down Expand Up @@ -18640,9 +18643,8 @@
if (el.hasAttribute("data-shiny-no-bind-input"))
return "continue";
var id = binding.getId(el);
if (!id)
if (!id || (0, import_jquery37.default)(el).hasClass("shiny-bound-input"))
return "continue";
bindingsRegistery.addBinding(id, "input");
var type = binding.getType(el);
var effectiveId = type ? id + ":" + type : id;
inputItems[effectiveId] = {
Expand All @@ -18667,6 +18669,7 @@
if (ratePolicy !== null) {
inputsRate.setRatePolicy(effectiveId, ratePolicy.policy, ratePolicy.delay);
}
bindingsRegistry.addBinding(id, "input");
(0, import_jquery37.default)(el).trigger({
type: "shiny:bound",
binding: binding,
Expand All @@ -18689,7 +18692,7 @@
}
function _bindOutputs() {
_bindOutputs = _asyncToGenerator9(/* @__PURE__ */ _regeneratorRuntime9().mark(function _callee(_ref3) {
var sendOutputHiddenState, maybeAddThemeObserver, outputBindings, scope, $scope, bindings, i5, binding, matches, j3, _el2, id, $el, bindingAdapter, _args = arguments;
var sendOutputHiddenState, maybeAddThemeObserver, outputBindings, scope, $scope, bindings, i5, binding, matches, j3, _el2, _id3, $el, bindingAdapter, _args = arguments;
return _regeneratorRuntime9().wrap(function _callee$(_context) {
while (1)
switch (_context.prev = _context.next) {
Expand All @@ -18713,36 +18716,36 @@
break;
}
_el2 = matches[j3];
id = binding.getId(_el2);
if (id) {
_id3 = binding.getId(_el2);
if (_id3) {
_context.next = 14;
break;
}
return _context.abrupt("continue", 28);
case 14:
bindingsRegistery.addBinding(id, "output");
if (import_jquery37.default.contains(document.documentElement, _el2)) {
_context.next = 17;
_context.next = 16;
break;
}
return _context.abrupt("continue", 28);
case 17:
case 16:
$el = (0, import_jquery37.default)(_el2);
if (!$el.hasClass("shiny-bound-output")) {
_context.next = 20;
_context.next = 19;
break;
}
return _context.abrupt("continue", 28);
case 20:
case 19:
maybeAddThemeObserver(_el2);
bindingAdapter = new OutputBindingAdapter(_el2, binding);
_context.next = 24;
return shinyAppBindOutput(id, bindingAdapter);
case 24:
_context.next = 23;
return shinyAppBindOutput(_id3, bindingAdapter);
case 23:
$el.data("shiny-output-binding", bindingAdapter);
$el.addClass("shiny-bound-output");
if (!$el.attr("aria-live"))
$el.attr("aria-live", "polite");
bindingsRegistry.addBinding(_id3, "output");
$el.trigger({
type: "shiny:bound",
binding: binding,
Expand Down Expand Up @@ -18779,9 +18782,9 @@
var binding = (0, import_jquery37.default)(_el).data("shiny-input-binding");
if (!binding)
continue;
var id = binding.getId(_el);
var _id = binding.getId(_el);
(0, import_jquery37.default)(_el).removeClass("shiny-bound-input");
bindingsRegistery.removeBinding(id, "input");
bindingsRegistry.removeBinding(_id, "input");
binding.unsubscribe(_el);
(0, import_jquery37.default)(_el).trigger({
type: "shiny:unbound",
Expand All @@ -18803,9 +18806,9 @@
var bindingAdapter = $el.data("shiny-output-binding");
if (!bindingAdapter)
continue;
var id = bindingAdapter.binding.getId(outputs[i5]);
shinyAppUnbindOutput(id, bindingAdapter);
bindingsRegistery.removeBinding(id, "output");
var _id2 = bindingAdapter.binding.getId(outputs[i5]);
shinyAppUnbindOutput(_id2, bindingAdapter);
bindingsRegistry.removeBinding(_id2, "output");
$el.removeClass("shiny-bound-output");
$el.removeData("shiny-output-binding");
$el.trigger({
Expand All @@ -18831,7 +18834,7 @@
return bindOutputs(shinyCtx, scope);
case 2:
currentInputs = bindInputs(shinyCtx, scope);
bindingValidity = bindingsRegistery.checkValidity();
bindingValidity = bindingsRegistry.checkValidity();
if (!(bindingValidity.status === "error")) {
_context2.next = 6;
break;
Expand Down
6 changes: 3 additions & 3 deletions inst/www/shared/shiny.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion inst/www/shared/shiny.min.js
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions inst/www/shared/shiny.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

71 changes: 36 additions & 35 deletions srcts/src/shiny/bind.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ function valueChangeCallback(
* immediately invoked function to keep the sets private and not clutter the
* scope.
*/
const bindingsRegistery = (() => {
const bindingsRegistry = (() => {
/**
* Keyed by binding IDs to the array of each type of binding that ID is associated for in current app state.
*
Expand All @@ -61,31 +61,32 @@ const bindingsRegistery = (() => {
const bindings: IdToBindingTypes = new Map();

/**
* Checks if the bindings registery is valid. Currently this just checks for
* Checks if the bindings registry is valid. Currently this just checks for
* duplicate IDs but in the future could be expanded to check more conditions
* @returns ShinyClientError if current ID bindings are invalid, otherwise null
* @returns ShinyClientError if current ID bindings are invalid, otherwise
* returns an ok status.
*/
function checkValidity():
| { status: "error"; error: ShinyClientError }
| { status: "ok" } {
const duplicateIds: IdToBindingTypes = new Map();
const duplicateIds: { [id: string]: { input: number; output: number } } =
{};
gadenbuie marked this conversation as resolved.
Show resolved Hide resolved

bindings.forEach((inputOrOutput, id) => {
if (inputOrOutput.length > 1) {
duplicateIds.set(id, inputOrOutput);
// count duplicate IDs of each binding type
bindings.forEach((idTypes, id) => {
const nInputs = idTypes.filter((s) => s === "input").length;
const nOutputs = idTypes.filter((s) => s === "output").length;

gadenbuie marked this conversation as resolved.
Show resolved Hide resolved
if (nInputs > 1 || nOutputs > 1) {
duplicateIds[id] = { input: nInputs, output: nOutputs };
}
Copy link
Contributor

@nstrayer nstrayer Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only bit of logic that's different here? If it is I may advocate for reducing changes to just this to keep changes focused.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of, but we also have to count the number of each type earlier. Before you would add an id to duplicateIds if bindings.get(id).length > 1 but we need this logic to happen earlier so we can take into account whether there are more than one of the same type as the inclusion criteria.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the diff is probably cleaner with this approach, but I'm sure there are other ways to do this same thing and I'm obviously open to any suggestions.

});

if (duplicateIds.size === 0) return { status: "ok" };

const duplicateIdMsg = Array.from(duplicateIds.entries())
.map(([id, idTypes]) => {
const counts = { input: 0, output: 0 };

idTypes.forEach((idType) => {
counts[idType]++;
});
const nDuplicates = Object.keys(duplicateIds).length;
if (nDuplicates === 0) return { status: "ok" };

const duplicateIdMsg = Object.entries(duplicateIds)
.map(([id, counts]) => {
const messages = [
pluralize(counts.input, "input"),
pluralize(counts.output, "output"),
Expand All @@ -102,44 +103,44 @@ const bindingsRegistery = (() => {
error: new ShinyClientError({
headline: "Duplicate input/output IDs found",
message: `The following ${
duplicateIds.size === 1 ? "ID was" : "IDs were"
nDuplicates === 1 ? "ID was" : "IDs were"
} repeated:\n${duplicateIdMsg}`,
}),
};
}

/**
* Add a binding id to the binding ids registery
* Add a binding id to the binding ids registry
* @param id Id to add
* @param inputOrOutput Whether the id is for an input or output binding
* @param type Binding type, either "input" or "output"
*/
function addBinding(id: string, inputOrOutput: "input" | "output"): void {
function addBinding(id: string, type: "input" | "output"): void {
if (id === "") {
throw new ShinyClientError({
headline: `Empty ${inputOrOutput} ID found`,
headline: `Empty ${type} ID found`,
Copy link
Contributor

@nstrayer nstrayer Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm biased, but I think the more descriptive parameter/variable name is nice here as it's self-documenting and when using it you don't need to reference the typescript types to understand the possible values. If we think we may add more binding types in the future I could see making it more genericly type, however. Also, it's not explicitly related to the issue the PR is trying to solve.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched it to type because isRegistered() would take inputOrOutput: "all" | "input" | "output", which wasn't just input or output. I personally found inputOrOutput to be slightly confusing -- given the longer name it feels like there's something more going on with it. I don't have strong feelings and could easily revert it for a smaller diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the change suggested here, I'd advocate for bindingType instead of inputOrOutput. Then bindingType is used everywhere to mean the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in b124d1b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh BindingType and then ensuring consistency is better!

message: "Binding IDs must not be empty.",
});
}

const existingBinding = bindings.get(id);

if (existingBinding) {
existingBinding.push(inputOrOutput);
existingBinding.push(type);
} else {
bindings.set(id, [inputOrOutput]);
bindings.set(id, [type]);
}
}

/**
* Remove a binding id from the binding ids registery
* Remove a binding id from the binding ids registry
* @param id Id to remove
* @param inputOrOutput Whether the id is for an input or output binding
* @param type Binding type, either "input" or "output"
*/
function removeBinding(id: string, inputOrOutput: "input" | "output"): void {
function removeBinding(id: string, type: "input" | "output"): void {
const existingBinding = bindings.get(id);

if (existingBinding) {
const index = existingBinding.indexOf(inputOrOutput);
const index = existingBinding.indexOf(type);
if (index > -1) {
existingBinding.splice(index, 1);
}
Expand Down Expand Up @@ -204,10 +205,9 @@ function bindInputs(
if (el.hasAttribute("data-shiny-no-bind-input")) continue;
const id = binding.getId(el);

// Check if ID is falsy, skip
if (!id) continue;
// Don't bind if ID is falsy or is currently bound
if (!id || $(el).hasClass("shiny-bound-input")) continue;

bindingsRegistery.addBinding(id, "input");
const type = binding.getType(el);
const effectiveId = type ? id + ":" + type : id;

Expand Down Expand Up @@ -243,6 +243,7 @@ function bindInputs(
);
}

bindingsRegistry.addBinding(id, "input");
$(el).trigger({
type: "shiny:bound",
// @ts-expect-error; Can not remove info on a established, malformed Event object
Expand Down Expand Up @@ -279,8 +280,6 @@ async function bindOutputs(
// Check if ID is falsy
if (!id) continue;

bindingsRegistery.addBinding(id, "output");

Comment on lines -282 to -283
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few conditions in the lines below (not seen in the diff) where we would continue and not bind to el because it's already bound. If we add this id to the bindingsRegistry here, then we'll overcount the bindings on id.

// In some uncommon cases, elements that are later in the
// matches array can be removed from the document by earlier
// iterations. See https://github.com/rstudio/shiny/issues/1399
Expand All @@ -306,6 +305,8 @@ async function bindOutputs(
$el.data("shiny-output-binding", bindingAdapter);
$el.addClass("shiny-bound-output");
if (!$el.attr("aria-live")) $el.attr("aria-live", "polite");

bindingsRegistry.addBinding(id, "output");
$el.trigger({
type: "shiny:bound",
// @ts-expect-error; Can not remove info on a established, malformed Event object
Expand Down Expand Up @@ -341,7 +342,7 @@ function unbindInputs(

$(el).removeClass("shiny-bound-input");

bindingsRegistery.removeBinding(id, "input");
bindingsRegistry.removeBinding(id, "input");
binding.unsubscribe(el);
$(el).trigger({
type: "shiny:unbound",
Expand Down Expand Up @@ -373,7 +374,7 @@ function unbindOutputs(

shinyAppUnbindOutput(id, bindingAdapter);

bindingsRegistery.removeBinding(id, "output");
bindingsRegistry.removeBinding(id, "output");
$el.removeClass("shiny-bound-output");
$el.removeData("shiny-output-binding");
$el.trigger({
Expand Down Expand Up @@ -403,7 +404,7 @@ async function _bindAll(
// complete error message that contains everything they will need to fix. If
// we threw as we saw collisions then the user would fix the first collision,
// re-run, and then see the next collision, etc.
const bindingValidity = bindingsRegistery.checkValidity();
const bindingValidity = bindingsRegistry.checkValidity();
if (bindingValidity.status === "error") {
throw bindingValidity.error;
}
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1853,7 +1853,7 @@ __metadata:
peerDependenciesMeta:
jquery-ui:
optional: true
checksum: 8718ebda1068894fc1267459b603f492045723ed1000fdbe798f2fab78fed8536b1906f56c53e9bd0ff9dce24aed176045618d0a1eddcf48f7d0313ad4ad67e9
checksum: ba260ba5804c16b1455ff79f9d00ce860e12ae36e29d7a5f702da6b384c9454497421b8e06fe683d10fac53e2dc6ec008da4fa129a153cbbfe5396e027eb4247
languageName: node
linkType: hard

Expand Down
Loading