From f400b97e0097a1fec7c32dcbb13d23d727661106 Mon Sep 17 00:00:00 2001 From: Isaiah Thomason <47364027+ITenthusiasm@users.noreply.github.com> Date: Tue, 16 Apr 2024 08:03:15 -0400 Subject: [PATCH] fix: Don't Mistake Renderable Error Objects as `ErrorDetails` Objects Our new `renderByDefault` feature left open a bug where someone might intend to render an object for their error message, but the object would be mistaken for an `ErrorDetails` object instead (or a `ErrorDetails` object). Now, we consider an object to be an `ErrorDetails` object if it's an object that explicitly has the `message` property (since that property is always required). Otherwise, we consider objects to be renderable error messages (assuming that rendering is enabled). The TypeScript types that we added previously should also help people avoid any unexpected bugs here. --- .../integrations/README.md | 6 +-- packages/core/FormValidityObserver.js | 2 +- .../__tests__/FormValidityObserver.test.ts | 43 ++++++++++++++++++- .../createFormValidityObserver.test.tsx | 21 +++++++++ packages/preact/createFormValidityObserver.js | 2 +- .../createFormValidityObserver.test.tsx | 21 +++++++++ packages/react/createFormValidityObserver.js | 2 +- .../createFormValidityObserver.test.tsx | 21 +++++++++ packages/solid/createFormValidityObserver.js | 2 +- .../createFormValidityObserver.test.ts | 21 +++++++++ packages/svelte/createFormValidityObserver.js | 2 +- .../createFormValidityObserver.test.ts | 21 +++++++++ packages/vue/createFormValidityObserver.js | 2 +- 13 files changed, 156 insertions(+), 10 deletions(-) diff --git a/docs/form-validity-observer/integrations/README.md b/docs/form-validity-observer/integrations/README.md index 5596431..a1e2876 100644 --- a/docs/form-validity-observer/integrations/README.md +++ b/docs/form-validity-observer/integrations/README.md @@ -459,7 +459,7 @@ export default function createFormValidityObserver< /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof errorMessages[constraint] !== "object") { + if (typeof errorMessages[constraint] !== "object" || !("message" in errorMessages[constraint])) { if (constraint === "required" && typeof errorMessages[constraint] !== "boolean") { config[constraint] = errorMessages[constraint]; } @@ -491,7 +491,7 @@ Here in `configure`, we're looping over each of the properties provided in the ` 1. If the constraint _value_ is `null` or `undefined`, then the constraint was omitted by the developer. There is nothing to add to the local error `config` or the returned constraint `props`. A `required` constraint with a value of `false` is treated as if it was `undefined`. 2. If the _constraint_ is `badinput` or `validate`, it can be copied directly to the error `config`. There are no `props` to update here since `badinput` and `validate` are not valid HTML attributes. 3. If the constraint _value_ is not a `SvelteErrorDetails` object, then we can assume that we have a raw constraint value. (For instance, we could have a raw `number` value for the `max` constraint.) The developer has indicated that they want to specify a field constraint without a custom error message; so only the constraint `props` are updated.

The exception to this rule is the `required` constraint. If the _constraint_ is `required` **and** the constraint _value_ is an `ErrorMessage`, then we assign this value to the error `config` instead of the `props` object. In this scenario, the _value_ for the `required` constraint is implicitly `true` (even if the value is an empty string).

-4. If the constraint _value_ is a `SvelteErrorDetails` object, then we can give the `value` property on this object to the `props` object. For simplicity, the error `config` can be given the entire constraint object in this scenario, even though it won't use the attached `value` property. Notice also that here, yet again, a `required` constraint with a value of `false` is treated as if the constraint was `undefined`. +4. If the constraint _value_ is a `SvelteErrorDetails` object (determined by the existence of a `message` property in the object), then we can give the `value` property on this object to the `props` object. For simplicity, the error `config` can be given the entire constraint object in this scenario, even though it won't use the attached `value` property. Notice also that here, yet again, a `required` constraint with a value of `false` is treated as if the constraint was `undefined`. After we finish looping over the properties in `errorMessages`, we configure the error messages for the field by calling the _core_ `FormValidityObserver.configure()` method with the error `config` object. Finally, we return any necessary form field `props`. @@ -553,7 +553,7 @@ observer.configure = (name, errorMessages) => { /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof constraintValue !== "object") { + if (typeof constraintValue !== "object" || !("message" in constraintValue)) { if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue; props[constraint] = constraint === "required" ? true : constraintValue; continue; diff --git a/packages/core/FormValidityObserver.js b/packages/core/FormValidityObserver.js index 3a0892f..c8bca2e 100644 --- a/packages/core/FormValidityObserver.js +++ b/packages/core/FormValidityObserver.js @@ -346,7 +346,7 @@ class FormValidityObserver extends FormObserver { return true; } - if (typeof error === "object") { + if (typeof error === "object" && "message" in error) { this.setFieldError(field.name, /** @type {any} */ (error).message, /** @type {any} */ (error).render); } else this.setFieldError(field.name, /** @type {any} */ (error)); diff --git a/packages/core/__tests__/FormValidityObserver.test.ts b/packages/core/__tests__/FormValidityObserver.test.ts index 7ee3372..5b5f70d 100644 --- a/packages/core/__tests__/FormValidityObserver.test.ts +++ b/packages/core/__tests__/FormValidityObserver.test.ts @@ -1807,7 +1807,6 @@ describe("Form Validity Observer (Class)", () => { const formValidityObserver = new FormValidityObserver(types, { renderer }); formValidityObserver.observe(form); - vi.spyOn(formValidityObserver, "setFieldError"); const message = Infinity; formValidityObserver.configure(field.name, { @@ -1932,6 +1931,48 @@ describe("Form Validity Observer (Class)", () => { expect(formValidityObserver.clearFieldError).not.toHaveBeenCalled(); expect(namelessField).toHaveAttribute(attrs["aria-invalid"], String(true)); }); + + describe("Bug Fixes", () => { + it("Does not mistake renderable error message objects for `ErrorDetails` objects", () => { + /* ---------- Setup ---------- */ + // Render Field + const { form, field } = renderField(createElementWithProps("input", { name: "objects", required: true })); + renderErrorContainerForField(field); + + // Setup `FormValidityObserver` + type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string }; + const renderer = vi.fn((errorContainer: HTMLElement, error: StringOrElement | null) => { + if (error === null) return errorContainer.replaceChildren(); + errorContainer.replaceChildren(error.value); + }); + + const formValidityObserver = new FormValidityObserver(types, { renderer, renderByDefault: true }); + formValidityObserver.observe(form); + + const stringMessage = "I am a bad string, bro..."; + const elementMessage = document.createElement("div"); + elementMessage.textContent = "I'm all alone!!!"; + + formValidityObserver.configure(field.name, { + // @ts-expect-error -- `render` should get ignored here because no `message` property is present + required: { type: "DOMElement", value: elementMessage, render: false }, + validate: () => ({ message: stringMessage, render: false }), + }); + + /* ---------- Run Assertions ---------- */ + // Trigger `required` error + expect(formValidityObserver.validateField(field.name)).toBe(false); + expectErrorFor(field, elementMessage.textContent, "html"); + expect(renderer).toHaveBeenCalledTimes(1); + + // Trigger User-Defined Error + field.value = "`required` is Satisfied"; // Avoid triggering events + + expect(formValidityObserver.validateField(field.name)).toBe(false); + expectErrorFor(field, stringMessage, "a11y"); + expect(renderer).toHaveBeenCalledTimes(1); // `renderer` wasn't used this time + }); + }); }); describe("validateFields (Method)", () => { diff --git a/packages/preact/__tests__/createFormValidityObserver.test.tsx b/packages/preact/__tests__/createFormValidityObserver.test.tsx index 0d919fa..9607e6a 100644 --- a/packages/preact/__tests__/createFormValidityObserver.test.tsx +++ b/packages/preact/__tests__/createFormValidityObserver.test.tsx @@ -304,6 +304,27 @@ describe("Create Form Validity Observer (Function)", () => { expect(observer.configure(name, {})).toStrictEqual({ name }); expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {}); }); + + describe("Bug Fixes", () => { + it("Does not mistake renderable error message objects for `PreactErrorDetails` objects", () => { + // Setup + type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string }; + const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined; + + vi.spyOn(FormValidityObserver.prototype, "configure"); + const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true }); + + // Test a Renderable Error Message + const renderable = { type: "DOMString", value: "No" } as const; + expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable }); + + // Test an `ErrorDetails` Object + const errorDetails = { message: renderable, value: true } as const; + expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails }); + }); + }); }); }); }); diff --git a/packages/preact/createFormValidityObserver.js b/packages/preact/createFormValidityObserver.js index 69372f8..6d3fc69 100644 --- a/packages/preact/createFormValidityObserver.js +++ b/packages/preact/createFormValidityObserver.js @@ -78,7 +78,7 @@ export default function createFormValidityObserver(types, options) { /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof constraintValue !== "object") { + if (typeof constraintValue !== "object" || !("message" in constraintValue)) { if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue; props[constraint] = constraint === "required" ? true : constraintValue; continue; diff --git a/packages/react/__tests__/createFormValidityObserver.test.tsx b/packages/react/__tests__/createFormValidityObserver.test.tsx index c57d0e0..a75a1be 100644 --- a/packages/react/__tests__/createFormValidityObserver.test.tsx +++ b/packages/react/__tests__/createFormValidityObserver.test.tsx @@ -244,6 +244,27 @@ describe("Create Form Validity Observer (Function)", () => { expect(observer.configure(name, {})).toStrictEqual({ name }); expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {}); }); + + describe("Bug Fixes", () => { + it("Does not mistake renderable error message objects for `ReactErrorDetails` objects", () => { + // Setup + type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string }; + const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined; + + vi.spyOn(FormValidityObserver.prototype, "configure"); + const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true }); + + // Test a Renderable Error Message + const renderable = { type: "DOMString", value: "No" } as const; + expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable }); + + // Test an `ErrorDetails` Object + const errorDetails = { message: renderable, value: true } as const; + expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails }); + }); + }); }); }); }); diff --git a/packages/react/createFormValidityObserver.js b/packages/react/createFormValidityObserver.js index 292c5b7..6ad080e 100644 --- a/packages/react/createFormValidityObserver.js +++ b/packages/react/createFormValidityObserver.js @@ -96,7 +96,7 @@ export default function createFormValidityObserver(types, options) { /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof constraintValue !== "object") { + if (typeof constraintValue !== "object" || !("message" in constraintValue)) { if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue; props[constraintsMap[constraint]] = constraint === "required" ? true : constraintValue; continue; diff --git a/packages/solid/__tests__/createFormValidityObserver.test.tsx b/packages/solid/__tests__/createFormValidityObserver.test.tsx index 4dc11ba..3a1d83d 100644 --- a/packages/solid/__tests__/createFormValidityObserver.test.tsx +++ b/packages/solid/__tests__/createFormValidityObserver.test.tsx @@ -309,6 +309,27 @@ describe("Create Form Validity Observer (Function)", () => { expect(observer.configure(name, {})).toStrictEqual({ name }); expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {}); }); + + describe("Bug Fixes", () => { + it("Does not mistake renderable error message objects for `SolidErrorDetails` objects", () => { + // Setup + type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string }; + const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined; + + vi.spyOn(FormValidityObserver.prototype, "configure"); + const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true }); + + // Test a Renderable Error Message + const renderable = { type: "DOMString", value: "No" } as const; + expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable }); + + // Test an `ErrorDetails` Object + const errorDetails = { message: renderable, value: true } as const; + expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails }); + }); + }); }); }); }); diff --git a/packages/solid/createFormValidityObserver.js b/packages/solid/createFormValidityObserver.js index d3e3e8a..c7b64db 100644 --- a/packages/solid/createFormValidityObserver.js +++ b/packages/solid/createFormValidityObserver.js @@ -69,7 +69,7 @@ export default function createFormValidityObserver(types, options) { /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof constraintValue !== "object") { + if (typeof constraintValue !== "object" || !("message" in constraintValue)) { if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue; props[constraint] = constraint === "required" ? true : constraintValue; continue; diff --git a/packages/svelte/__tests__/createFormValidityObserver.test.ts b/packages/svelte/__tests__/createFormValidityObserver.test.ts index d9a9d74..8d22cc6 100644 --- a/packages/svelte/__tests__/createFormValidityObserver.test.ts +++ b/packages/svelte/__tests__/createFormValidityObserver.test.ts @@ -233,6 +233,27 @@ describe("Create Form Validity Observer (Function)", () => { expect(observer.configure(name, {})).toStrictEqual({ name }); expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {}); }); + + describe("Bug Fixes", () => { + it("Does not mistake renderable error message objects for `SvelteErrorDetails` objects", () => { + // Setup + type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string }; + const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined; + + vi.spyOn(FormValidityObserver.prototype, "configure"); + const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true }); + + // Test a Renderable Error Message + const renderable = { type: "DOMString", value: "No" } as const; + expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable }); + + // Test an `ErrorDetails` Object + const errorDetails = { message: renderable, value: true } as const; + expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails }); + }); + }); }); }); }); diff --git a/packages/svelte/createFormValidityObserver.js b/packages/svelte/createFormValidityObserver.js index 9f2e750..c961c5c 100644 --- a/packages/svelte/createFormValidityObserver.js +++ b/packages/svelte/createFormValidityObserver.js @@ -71,7 +71,7 @@ export default function createFormValidityObserver(types, options) { /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof constraintValue !== "object") { + if (typeof constraintValue !== "object" || !("message" in constraintValue)) { if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue; props[constraint] = constraint === "required" ? true : constraintValue; continue; diff --git a/packages/vue/__tests__/createFormValidityObserver.test.ts b/packages/vue/__tests__/createFormValidityObserver.test.ts index a844d4f..d1de644 100644 --- a/packages/vue/__tests__/createFormValidityObserver.test.ts +++ b/packages/vue/__tests__/createFormValidityObserver.test.ts @@ -245,6 +245,27 @@ describe("Create Form Validity Observer (Function)", () => { expect(observer.configure(name, {})).toStrictEqual({ name }); expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {}); }); + + describe("Bug Fixes", () => { + it("Does not mistake renderable error message objects for `VueErrorDetails` objects", () => { + // Setup + type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string }; + const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined; + + vi.spyOn(FormValidityObserver.prototype, "configure"); + const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true }); + + // Test a Renderable Error Message + const renderable = { type: "DOMString", value: "No" } as const; + expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable }); + + // Test an `ErrorDetails` Object + const errorDetails = { message: renderable, value: true } as const; + expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true }); + expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails }); + }); + }); }); }); }); diff --git a/packages/vue/createFormValidityObserver.js b/packages/vue/createFormValidityObserver.js index 49707e7..06dfa79 100644 --- a/packages/vue/createFormValidityObserver.js +++ b/packages/vue/createFormValidityObserver.js @@ -78,7 +78,7 @@ export default function createFormValidityObserver(types, options) { /* ----- Standrd HTML Attributes ----- */ // Value Only - if (typeof constraintValue !== "object") { + if (typeof constraintValue !== "object" || !("message" in constraintValue)) { if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue; props[constraint] = constraint === "required" ? true : constraintValue; continue;