Skip to content

Commit

Permalink
feat: Avoid throwing errors for shared input/output IDs (#4101)
Browse files Browse the repository at this point in the history
* refactor: Factor out message display from error handler

* feat: Add custom event for sending a client message

* feat: Report binding validity problem via event instead of throwing error

* feat: Don't need to hide shared input/output message

Now that it's not an error, it's safe to report

* refactor: Move `inDevMode()` logic into error console

* refactor: Rename `.error` --> `.event`

* feat: wrap client error message

It's otherwise hard to tell that the error is scrollable
Plus the scrolling is over the whole message rather than the part that overflows

* feat: always send client console messages to browser console as well

* chore: throw if `shiny:client-message` receives an event that isn't CustomEvent

* feat: Handle status in `showShinyClientMessage()`

* Renamed `showMessageInClientConsole()` to `showShinyClientMessage()` to improve clarity

* Added `status` argument to `showShinyClientMessage()` to allow for different message types

* refactor: Don't throw errors for duplicate IDs

Brings dev mode in line with current "prod" behavior,
where errors aren't thrown for duplciates. In both cases
we still get console or client messages.

* refactor: Clean up `status` inside `checkValidity()`

* refactor: Have `checkValidity()` handle emitting the client console event
gadenbuie authored Dec 6, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent ce6a562 commit e5083f4
Showing 8 changed files with 485 additions and 322 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -22,6 +22,8 @@

* Fixed a bug with stack trace capturing that caused reactives with very long async promise chains (hundreds/thousands of steps) to become extremely slow. Chains this long are unlikely to be written by hand, but {coro} async generators and {elmer} async streaming were easily creating problematically long chains. (#4155)

* Duplicate input and output IDs -- e.g. using `"debug"` for two inputs or two outputs -- or shared IDs -- e.g. using `"debug"` as the `inputID` for an input and an output -- now result in a console warning message, but not an error. When `devmode()` is enabled, an informative message is shown in the Shiny Client Console. We recommend all Shiny devs enable `devmode()` when developing Shiny apps locally. (#4101)

* Updating the choices of a `selectizeInput()` via `updateSelectizeInput()` with `server = TRUE` no longer retains the selected choice as a deselected option if the current value is not part of the new choices. (@dvg-p4 #4142)

# shiny 1.9.1
617 changes: 355 additions & 262 deletions inst/www/shared/shiny.js

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions inst/www/shared/shiny.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion inst/www/shared/shiny.min.js

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions inst/www/shared/shiny.min.js.map

Large diffs are not rendered by default.

83 changes: 66 additions & 17 deletions srcts/src/components/errorConsole.ts
Original file line number Diff line number Diff line change
@@ -307,6 +307,7 @@ export class ShinyErrorMessage extends LitElement {
.error-message {
font-family: "Courier New", Courier, monospace;
white-space: pre-wrap;
}
.decoration-container {
@@ -489,6 +490,52 @@ export class ShinyErrorMessage extends LitElement {

customElements.define("shiny-error-message", ShinyErrorMessage);

export type ShinyClientMessage = {
message: string;
headline?: string;
status?: "error" | "info" | "warning";
};

function showShinyClientMessage({
headline = "",
message,
status = "warning",
}: ShinyClientMessage): void {
const consoleMessage = `[shiny] ${headline}${
headline ? " - " : ""
}${message}`;

switch (status) {
case "error":
console.error(consoleMessage);
break;
case "warning":
console.warn(consoleMessage);
break;
default:
console.log(consoleMessage);
break;
}

if (!Shiny.inDevMode()) {
return;
}

// Check to see if an Error Console Container element already exists. If it
// doesn't we need to add it before putting an error on the screen
let errorConsoleContainer = document.querySelector("shiny-error-console");
if (!errorConsoleContainer) {
errorConsoleContainer = document.createElement("shiny-error-console");
document.body.appendChild(errorConsoleContainer);
}

const errorConsole = document.createElement("shiny-error-message");
errorConsole.setAttribute("headline", headline);
errorConsole.setAttribute("message", message);

errorConsoleContainer.appendChild(errorConsole);
}

/**
* Function to show an error message to user in shiny-error-message web
* component. Only shows the error if we're in development mode.
@@ -497,11 +544,6 @@ customElements.define("shiny-error-message", ShinyErrorMessage);
* object.
*/
export function showErrorInClientConsole(e: unknown): void {
if (!Shiny.inDevMode()) {
// If we're in production, don't show the error to the user
return;
}

let errorMsg: string | null = null;
let headline = "Error on client while running Shiny app";

@@ -516,17 +558,24 @@ export function showErrorInClientConsole(e: unknown): void {
errorMsg = "Unknown error";
}

// Check to see if an Error Console Container element already exists. If it
// doesn't we need to add it before putting an error on the screen
let errorConsoleContainer = document.querySelector("shiny-error-console");
if (!errorConsoleContainer) {
errorConsoleContainer = document.createElement("shiny-error-console");
document.body.appendChild(errorConsoleContainer);
}

const errorConsole = document.createElement("shiny-error-message");
errorConsole.setAttribute("headline", headline || "");
errorConsole.setAttribute("message", errorMsg);
showShinyClientMessage({ headline, message: errorMsg, status: "error" });
}

errorConsoleContainer.appendChild(errorConsole);
export class ShinyClientMessageEvent extends CustomEvent<ShinyClientMessage> {
constructor(detail: ShinyClientMessage) {
super("shiny:client-message", { detail, bubbles: true, cancelable: true });
}
}

window.addEventListener("shiny:client-message", (ev: Event) => {
if (!(ev instanceof CustomEvent)) {
throw new Error("[shiny] shiny:client-message expected a CustomEvent");
}
const { headline, message, status } = ev.detail;
if (!message) {
throw new Error(
"[shiny] shiny:client-message expected a `message` property in `event.detail`."
);
}
showShinyClientMessage({ headline, message, status });
});
79 changes: 45 additions & 34 deletions srcts/src/shiny/bind.ts
Original file line number Diff line number Diff line change
@@ -3,11 +3,11 @@ import { Shiny } from "..";
import type { InputBinding, OutputBinding } from "../bindings";
import { OutputBindingAdapter } from "../bindings/outputAdapter";
import type { BindingRegistry } from "../bindings/registry";
import { ShinyClientMessageEvent } from "../components/errorConsole";
import type {
InputRateDecorator,
InputValidateDecorator,
} from "../inputPolicies";
import { ShinyClientError } from "./error";
import { shinyAppBindOutput, shinyAppUnbindOutput } from "./initedMethods";
import { sendImageSizeFns } from "./sendImageSize";

@@ -75,37 +75,44 @@ const bindingsRegistry = (() => {
* accessibility and other reasons. However, in practice our bindings still
* work as long as inputs the IDs within a binding type don't overlap.
*
* @returns ShinyClientError if current ID bindings are invalid, otherwise
* returns an ok status.
* @returns ShinyClientMessageEvent if current ID bindings are invalid,
* otherwise returns an ok status.
*/
function checkValidity():
| { status: "error"; error: ShinyClientError }
| { status: "ok" } {
function checkValidity(scope: BindScope): void {
type BindingCounts = { [T in BindingTypes]: number };
const duplicateIds = new Map<string, BindingCounts>();
const problems: Set<string> = new Set();

// count duplicate IDs of each binding type
bindings.forEach((idTypes, id) => {
const counts: { [T in BindingTypes]: number } = { input: 0, output: 0 };

idTypes.forEach((type) => (counts[type] += 1));

// If there's a single duplication of ids across both binding types, then
// when we're not in devmode, we allow this to pass because a good amount of
// existing applications use this pattern even though its invalid. Eventually
// this behavior should be removed.
if (counts.input === 1 && counts.output === 1 && !Shiny.inDevMode()) {
if (counts.input + counts.output < 2) {
return;
}
// We have duplicated IDs, add them to the set of duplicated IDs to be
// reported to the user.
duplicateIds.set(id, counts);

// If we have duplicated IDs, then add them to the set of duplicated IDs
// to be reported to the user.
if (counts.input + counts.output > 1) {
duplicateIds.set(id, counts);
if (counts.input > 1) {
problems.add("input");
}
if (counts.output > 1) {
problems.add("output");
}
if (counts.input >= 1 && counts.output >= 1) {
problems.add("shared");
}
});

if (duplicateIds.size === 0) return { status: "ok" };
if (duplicateIds.size === 0) return;
// Duplicated IDs are now always a warning. Before the ShinyClient console
// was added duplicate output IDs were errors in "production" mode. After
// the Shiny Client console was introduced, duplicate IDs were no longer
// production errors but *would* break apps in dev mode. Now, in v1.10+,
// duplicate IDs are always warnings in all modes for consistency.

const duplicateIdMsg = Array.from(duplicateIds.entries())
.map(([id, counts]) => {
@@ -120,15 +127,27 @@ const bindingsRegistry = (() => {
})
.join("\n");

return {
status: "error",
error: new ShinyClientError({
headline: "Duplicate input/output IDs found",
message: `The following ${
duplicateIds.size === 1 ? "ID was" : "IDs were"
} repeated:\n${duplicateIdMsg}`,
}),
};
let txtVerb = "Duplicate";
let txtNoun = "input/output";
if (problems.has("input") && problems.has("output")) {
// base case
} else if (problems.has("input")) {
txtNoun = "input";
} else if (problems.has("output")) {
txtNoun = "output";
} else if (problems.has("shared")) {
txtVerb = "Shared";
}

const txtIdsWere = duplicateIds.size == 1 ? "ID was" : "IDs were";
const headline = `${txtVerb} ${txtNoun} ${txtIdsWere} found`;
const message = `The following ${txtIdsWere} used for more than one ${
problems.has("shared") ? "input/output" : txtNoun
}:\n${duplicateIdMsg}`;

const event = new ShinyClientMessageEvent({ headline, message });
const scopeElement = scope instanceof HTMLElement ? scope : scope.get(0);
(scopeElement || window).dispatchEvent(event);
}

/**
@@ -423,15 +442,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 = bindingsRegistry.checkValidity();
if (bindingValidity.status === "error") {
// Only throw if we're in dev mode. Otherwise, just log a warning.
if (Shiny.inDevMode()) {
throw bindingValidity.error;
} else {
console.warn("[shiny] " + bindingValidity.error.message);
}
}
bindingsRegistry.checkValidity(scope);

return currentInputs;
}
8 changes: 8 additions & 0 deletions srcts/types/src/components/errorConsole.d.ts
Original file line number Diff line number Diff line change
@@ -10,6 +10,11 @@ export declare class ShinyErrorMessage extends LitElement {
copyErrorToClipboard(): Promise<void>;
render(): import("lit-html").TemplateResult<1>;
}
export type ShinyClientMessage = {
message: string;
headline?: string;
status?: "error" | "info" | "warning";
};
/**
* Function to show an error message to user in shiny-error-message web
* component. Only shows the error if we're in development mode.
@@ -18,3 +23,6 @@ export declare class ShinyErrorMessage extends LitElement {
* object.
*/
export declare function showErrorInClientConsole(e: unknown): void;
export declare class ShinyClientMessageEvent extends CustomEvent<ShinyClientMessage> {
constructor(detail: ShinyClientMessage);
}

0 comments on commit e5083f4

Please sign in to comment.