Skip to content

Commit 7e2f30b

Browse files
ITenthusiasmITenthusiasm
committed
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 `<FRAMEWORK>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.
1 parent 7f49940 commit 7e2f30b

13 files changed

+157
-11
lines changed

‎docs/form-validity-observer/integrations/README.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ export default function createFormValidityObserver<
459459
460460
/* ----- Standrd HTML Attributes ----- */
461461
// Value Only
462-
if (typeof errorMessages[constraint] !== "object") {
462+
if (typeof errorMessages[constraint] !== "object" || !("message" in errorMessages[constraint])) {
463463
if (constraint === "required" && typeof errorMessages[constraint] !== "boolean") {
464464
config[constraint] = errorMessages[constraint];
465465
}
@@ -489,9 +489,9 @@ If you're encountering TypeScript errors with the code above, we'll address that
489489
Here in `configure`, we're looping over each of the properties provided in the `errorMessages` object so that we can A&rpar; Derive the error configuration that needs to be passed to the _original_ `FormValidityObserver.configure()` method, and B&rpar; Derive the field props that need to be returned from the _enhanced_ `configure` method. Hopefully, from the code and the comments, it's clear why the code is written as it is. But in case things aren't clear, here's a summary:
490490
491491
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`.
492-
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.
492+
2. If the _constraint_ is `badinput` or `validate`, then its _value_ can be copied directly to the error `config`. There are no `props` to update here since `badinput` and `validate` are not valid HTML attributes.
493493
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. <p>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).</p>
494-
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`.
494+
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`.
495495
496496
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`.
497497
@@ -553,7 +553,7 @@ observer.configure = (name, errorMessages) => {
553553
554554
/* ----- Standrd HTML Attributes ----- */
555555
// Value Only
556-
if (typeof constraintValue !== "object") {
556+
if (typeof constraintValue !== "object" || !("message" in constraintValue)) {
557557
if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue;
558558
props[constraint] = constraint === "required" ? true : constraintValue;
559559
continue;

‎packages/core/FormValidityObserver.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,7 @@ class FormValidityObserver extends FormObserver {
346346
return true;
347347
}
348348

349-
if (typeof error === "object") {
349+
if (typeof error === "object" && "message" in error) {
350350
this.setFieldError(field.name, /** @type {any} */ (error).message, /** @type {any} */ (error).render);
351351
} else this.setFieldError(field.name, /** @type {any} */ (error));
352352

‎packages/core/__tests__/FormValidityObserver.test.ts

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1807,7 +1807,6 @@ describe("Form Validity Observer (Class)", () => {
18071807

18081808
const formValidityObserver = new FormValidityObserver(types, { renderer });
18091809
formValidityObserver.observe(form);
1810-
vi.spyOn(formValidityObserver, "setFieldError");
18111810

18121811
const message = Infinity;
18131812
formValidityObserver.configure(field.name, {
@@ -1932,6 +1931,48 @@ describe("Form Validity Observer (Class)", () => {
19321931
expect(formValidityObserver.clearFieldError).not.toHaveBeenCalled();
19331932
expect(namelessField).toHaveAttribute(attrs["aria-invalid"], String(true));
19341933
});
1934+
1935+
describe("Bug Fixes", () => {
1936+
it("Does not mistake renderable error message objects for `ErrorDetails` objects", () => {
1937+
/* ---------- Setup ---------- */
1938+
// Render Field
1939+
const { form, field } = renderField(createElementWithProps("input", { name: "objects", required: true }));
1940+
renderErrorContainerForField(field);
1941+
1942+
// Setup `FormValidityObserver`
1943+
type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string };
1944+
const renderer = vi.fn((errorContainer: HTMLElement, error: StringOrElement | null) => {
1945+
if (error === null) return errorContainer.replaceChildren();
1946+
errorContainer.replaceChildren(error.value);
1947+
});
1948+
1949+
const formValidityObserver = new FormValidityObserver(types, { renderer, renderByDefault: true });
1950+
formValidityObserver.observe(form);
1951+
1952+
const stringMessage = "I am a bad string, bro...";
1953+
const elementMessage = document.createElement("div");
1954+
elementMessage.textContent = "I'm all alone!!!";
1955+
1956+
formValidityObserver.configure(field.name, {
1957+
// @ts-expect-error -- `render` should get ignored here because no `message` property is present
1958+
required: { type: "DOMElement", value: elementMessage, render: false },
1959+
validate: () => ({ message: stringMessage, render: false }),
1960+
});
1961+
1962+
/* ---------- Run Assertions ---------- */
1963+
// Trigger `required` error
1964+
expect(formValidityObserver.validateField(field.name)).toBe(false);
1965+
expectErrorFor(field, elementMessage.textContent, "html");
1966+
expect(renderer).toHaveBeenCalledTimes(1);
1967+
1968+
// Trigger User-Defined Error
1969+
field.value = "`required` is Satisfied"; // Avoid triggering events
1970+
1971+
expect(formValidityObserver.validateField(field.name)).toBe(false);
1972+
expectErrorFor(field, stringMessage, "a11y");
1973+
expect(renderer).toHaveBeenCalledTimes(1); // `renderer` wasn't used this time
1974+
});
1975+
});
19351976
});
19361977

19371978
describe("validateFields (Method)", () => {

‎packages/preact/__tests__/createFormValidityObserver.test.tsx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,27 @@ describe("Create Form Validity Observer (Function)", () => {
304304
expect(observer.configure(name, {})).toStrictEqual({ name });
305305
expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {});
306306
});
307+
308+
describe("Bug Fixes", () => {
309+
it("Does not mistake renderable error message objects for `PreactErrorDetails` objects", () => {
310+
// Setup
311+
type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string };
312+
const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined;
313+
314+
vi.spyOn(FormValidityObserver.prototype, "configure");
315+
const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true });
316+
317+
// Test a Renderable Error Message
318+
const renderable = { type: "DOMString", value: "No" } as const;
319+
expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true });
320+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable });
321+
322+
// Test an `ErrorDetails` Object
323+
const errorDetails = { message: renderable, value: true } as const;
324+
expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true });
325+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails });
326+
});
327+
});
307328
});
308329
});
309330
});

‎packages/preact/createFormValidityObserver.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ export default function createFormValidityObserver(types, options) {
7878

7979
/* ----- Standrd HTML Attributes ----- */
8080
// Value Only
81-
if (typeof constraintValue !== "object") {
81+
if (typeof constraintValue !== "object" || !("message" in constraintValue)) {
8282
if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue;
8383
props[constraint] = constraint === "required" ? true : constraintValue;
8484
continue;

‎packages/react/__tests__/createFormValidityObserver.test.tsx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,27 @@ describe("Create Form Validity Observer (Function)", () => {
244244
expect(observer.configure(name, {})).toStrictEqual({ name });
245245
expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {});
246246
});
247+
248+
describe("Bug Fixes", () => {
249+
it("Does not mistake renderable error message objects for `ReactErrorDetails` objects", () => {
250+
// Setup
251+
type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string };
252+
const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined;
253+
254+
vi.spyOn(FormValidityObserver.prototype, "configure");
255+
const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true });
256+
257+
// Test a Renderable Error Message
258+
const renderable = { type: "DOMString", value: "No" } as const;
259+
expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true });
260+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable });
261+
262+
// Test an `ErrorDetails` Object
263+
const errorDetails = { message: renderable, value: true } as const;
264+
expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true });
265+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails });
266+
});
267+
});
247268
});
248269
});
249270
});

‎packages/react/createFormValidityObserver.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ export default function createFormValidityObserver(types, options) {
9696

9797
/* ----- Standrd HTML Attributes ----- */
9898
// Value Only
99-
if (typeof constraintValue !== "object") {
99+
if (typeof constraintValue !== "object" || !("message" in constraintValue)) {
100100
if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue;
101101
props[constraintsMap[constraint]] = constraint === "required" ? true : constraintValue;
102102
continue;

‎packages/solid/__tests__/createFormValidityObserver.test.tsx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,27 @@ describe("Create Form Validity Observer (Function)", () => {
309309
expect(observer.configure(name, {})).toStrictEqual({ name });
310310
expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {});
311311
});
312+
313+
describe("Bug Fixes", () => {
314+
it("Does not mistake renderable error message objects for `SolidErrorDetails` objects", () => {
315+
// Setup
316+
type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string };
317+
const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined;
318+
319+
vi.spyOn(FormValidityObserver.prototype, "configure");
320+
const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true });
321+
322+
// Test a Renderable Error Message
323+
const renderable = { type: "DOMString", value: "No" } as const;
324+
expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true });
325+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable });
326+
327+
// Test an `ErrorDetails` Object
328+
const errorDetails = { message: renderable, value: true } as const;
329+
expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true });
330+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails });
331+
});
332+
});
312333
});
313334
});
314335
});

‎packages/solid/createFormValidityObserver.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ export default function createFormValidityObserver(types, options) {
6969

7070
/* ----- Standrd HTML Attributes ----- */
7171
// Value Only
72-
if (typeof constraintValue !== "object") {
72+
if (typeof constraintValue !== "object" || !("message" in constraintValue)) {
7373
if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue;
7474
props[constraint] = constraint === "required" ? true : constraintValue;
7575
continue;

‎packages/svelte/__tests__/createFormValidityObserver.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,27 @@ describe("Create Form Validity Observer (Function)", () => {
233233
expect(observer.configure(name, {})).toStrictEqual({ name });
234234
expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {});
235235
});
236+
237+
describe("Bug Fixes", () => {
238+
it("Does not mistake renderable error message objects for `SvelteErrorDetails` objects", () => {
239+
// Setup
240+
type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string };
241+
const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined;
242+
243+
vi.spyOn(FormValidityObserver.prototype, "configure");
244+
const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true });
245+
246+
// Test a Renderable Error Message
247+
const renderable = { type: "DOMString", value: "No" } as const;
248+
expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true });
249+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable });
250+
251+
// Test an `ErrorDetails` Object
252+
const errorDetails = { message: renderable, value: true } as const;
253+
expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true });
254+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails });
255+
});
256+
});
236257
});
237258
});
238259
});

‎packages/svelte/createFormValidityObserver.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ export default function createFormValidityObserver(types, options) {
7171

7272
/* ----- Standrd HTML Attributes ----- */
7373
// Value Only
74-
if (typeof constraintValue !== "object") {
74+
if (typeof constraintValue !== "object" || !("message" in constraintValue)) {
7575
if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue;
7676
props[constraint] = constraint === "required" ? true : constraintValue;
7777
continue;

‎packages/vue/__tests__/createFormValidityObserver.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,27 @@ describe("Create Form Validity Observer (Function)", () => {
245245
expect(observer.configure(name, {})).toStrictEqual({ name });
246246
expect(FormValidityObserver.prototype.configure).toHaveBeenCalledWith(name, {});
247247
});
248+
249+
describe("Bug Fixes", () => {
250+
it("Does not mistake renderable error message objects for `VueErrorDetails` objects", () => {
251+
// Setup
252+
type StringOrElement = { type: "DOMElement"; value: HTMLElement } | { type: "DOMString"; value: string };
253+
const renderer = (_errorContainer: HTMLElement, _errorMessage: StringOrElement | null) => undefined;
254+
255+
vi.spyOn(FormValidityObserver.prototype, "configure");
256+
const observer = createFormValidityObserver(types[0], { renderer, renderByDefault: true });
257+
258+
// Test a Renderable Error Message
259+
const renderable = { type: "DOMString", value: "No" } as const;
260+
expect(observer.configure(name, { required: renderable })).toStrictEqual({ name, required: true });
261+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(1, name, { required: renderable });
262+
263+
// Test an `ErrorDetails` Object
264+
const errorDetails = { message: renderable, value: true } as const;
265+
expect(observer.configure(name, { required: errorDetails })).toStrictEqual({ name, required: true });
266+
expect(FormValidityObserver.prototype.configure).toHaveBeenNthCalledWith(2, name, { required: errorDetails });
267+
});
268+
});
248269
});
249270
});
250271
});

‎packages/vue/createFormValidityObserver.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ export default function createFormValidityObserver(types, options) {
7878

7979
/* ----- Standrd HTML Attributes ----- */
8080
// Value Only
81-
if (typeof constraintValue !== "object") {
81+
if (typeof constraintValue !== "object" || !("message" in constraintValue)) {
8282
if (constraint === "required" && typeof constraintValue !== "boolean") config[constraint] = constraintValue;
8383
props[constraint] = constraint === "required" ? true : constraintValue;
8484
continue;

0 commit comments

Comments
 (0)