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

[gadenbuie]

Fixes #3943
Fixes #3944
Fixes #3945

Prior to #3931, bindInputs() could be called more than once on the same input element and only the first binding would be applied. This behavior was changed in 62eb485

This PR makes a few changes to the implementation in #3931, plus one change to improve bindInputs() from the previous status quo:

1. bindInputs() once again no-ops if an attempt is made to bind to a currently bound input. Prior to Client error console and duplicate input/output binding errors #3931, we had a light-weight input binding registry and we would look to see if id was marked as bound, and no-op if it was in the registry. With Client error console and duplicate input/output binding errors #3931, we always re-bind the input element regardless of its binding status, most likely assuming that bindAll() was paired with a prior unbinding step at the right scope. This doesn't always play out in practice.

   With this PR, we now inspect the element directly. If it has the input binding data value that signifies it's currently bound, we do nothing -- after we make sure the element is included in the bindingsRegistry. (Inspecting the element directly is also what bindOutputs() does, so this bring bindInputs() in line with that function.)

2. For both the binding of inputs and outputs, I relocated the bindingsRegistry.addBinding() call to happen after Shiny performs the binding and before it announces the binding to the rest of the world via the shiny:bound event. There were a few places where we could unintentionally count a new binding when no actual binding was taking place.

3. I've updated the bindingsRegistry.checkValidity() method to only throw when an individual binding type has more than one binding. The lets input$caption and output$caption work as before, but if a second output$caption were added, the client error console will report all bindings and alert users. In the future we could update the error console to also handle warnings (so maybe we should call this the Shiny client message console?)

4. There are also a few additional miscellaneous fixes, the most salient of which is fixing the spelling of "registery" to "registry".

---

I've checked all of the shinycoreci apps affected by the error console (listed in rstudio/shinycoreci#248) and they all work as expected with the changes in this PR.

[jcheng5]

I think el.classList.contains("shiny-bound-input") is a better test. If I'm reading this correctly, .shiny-bound-input directly reflects whether this exact element is bound or not; whereas $(el).data("shiny-input-binding") will be truthy if this element has ever been bound.

[jcheng5]

And if you replace that test then you can simply continue without needing to conditionally call bindingsRegistry.addBinding(id, "input").

[gadenbuie]

Yeah I went this way because I knew it'd induce conversation about which approach is better.

shiny/srcts/src/shiny/bind.ts

Lines 404 to 405 in 3e00632

	$el.removeClass("shiny-bound-output");
	$el.removeData("shiny-output-binding");

unbindOutputs() should remove both the class and the data, so the presence of either should in theory be a solid signal. I'm inclined to lean toward the data value, since it's less likely a user would have included data-shiny-input-binding in the markup (but maybe they've added .shiny-bound-input to the class list for some reason?). If we really really want to be certain we could test the value of $(el).data("shiny-input-binding") but that seems excessive.

[gadenbuie]

Oh wait! Yes unbindInputs() leaves the data attribute in tact. So there is a currently correct answer of testing the class

shiny/srcts/src/shiny/bind.ts

Lines 368 to 370 in 3e00632

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

[gadenbuie]

I updated bindInputs() to check the class in 26e5109 and then removed the .isRegistered() method from bindingsRegistry since it isn't used anywhere else in b67b062.

[gadenbuie]

The change here would cause currently-bound inputs to both rebind and to increment the binding registry count, unless an intervening unbindAll() happened in between.

Which ultimately would conflate binding the same binding to the same element (it's already bound so the binding should no-op) with binding a new element with the same ID.

[gadenbuie]

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.

[nstrayer]

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.

[gadenbuie]

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.

[gadenbuie]

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

[gadenbuie]

Refactored in b124d1b

[nstrayer]

Oh BindingType and then ensuring consistency is better!

[nstrayer]

Just some thoughts on the changes related to not flagging duplicate ids when there's only one of each type.

[nstrayer]

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.

[gadenbuie]

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.

[gadenbuie]

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.

[nstrayer]

I'm mostly good with this.

The one thing I'm uneasy about is the fact that it explicitly allows the duplication of ids if they happen to be spread across a single pair of inputs and outputs. I definitely agree with the premise that we should not break legacy apps, but I think that by just building this fact into the code itself without an in-progress plan to fix it (or at least alerting the users of its non-idealness) then we risk codifying bad practices right into the codebase. At least previously this was a bug that we just happened to not catch.

One of the main purposes of the error console was to find exactly this type of long-standing bug that was never noticed before.

Maybe we should add a non-throwing warning message to the console? I'm curious to get @wch and @jcheng5 s opinions on this though as I very well may be being too dogmatic.

[gadenbuie]

I'm also interested in other's opinions, but I'd like to reiterate: the bad practice here is using the same id attribute for more than one DOM elements. IMHO, that is an implementation detail of shiny, not a concern for app authors. The problematic part is that we put the same id="foo" attribute on two different elements, not that app authors use input$foo and output$foo.

I've only found a few places in our documentation where we talk about inputs and outputs sharing a global namespace: both (article, package docs) are in sections related to Shiny modules. Neither are strongly worded or well-positioned enough to prevent this usage in real-world apps. And more importantly, the undefined behavior is theoretical, in practice our bindings work as intended in this situation.

IMHO, the first iteration of the error console should not break apps that currently work. This PR as it is now, covers both the current state of Shiny and the future world where we've changed how input and output IDs are stored on the bound elements.

I think augmenting the console to handle non-error messages is an excellent idea and would be broadly useful. That'd be great work to take up in a follow up PR.

[nstrayer]

I wonder if maybe the PR should be split into two parts:

1. Fixing the objectively broken behavior regarding the insertUI and removeUI binding logic and
2. fixing the more controversial shared ID problems.

Currently, the changes are a bit spread out. Personally, I think the fixes for 1 are great and ready to go and I think it would be good to let those fixes go through quickly without being dragged down by 2.

[wch]

Should we use the type that was defined above?

Suggested change

	const counts: { [T in BindingTypes]: number } = { input: 0, output: 0 };
	const counts: BindingCounts = { input: 0, output: 0 };

[gadenbuie]

Oh we should have removed the BindingCounts type since the definition above is more concise and would expand automatically if we added new binding types. Sorry, this change was made via the suggestion interface, so this was an oversight.

[wch]

Merging this now to fix the bug with insertUI, but I'll file an issue to come back to overlapping input/output IDs, and enabling this only for local development.