Skip to content

Commit

Permalink
fix: Don't Mistake Renderable Error Objects as ErrorDetails Objects
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ITenthusiasm committed Apr 17, 2024
1 parent 7f49940 commit 7e2f30b
Show file tree
Hide file tree
Showing 13 changed files with 157 additions and 11 deletions.
8 changes: 4 additions & 4 deletions docs/form-validity-observer/integrations/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
Expand Down Expand Up @@ -489,9 +489,9 @@ If you're encountering TypeScript errors with the code above, we'll address that
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:
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.
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.
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>
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`.
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion packages/core/FormValidityObserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
43 changes: 42 additions & 1 deletion packages/core/__tests__/FormValidityObserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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)", () => {
Expand Down
21 changes: 21 additions & 0 deletions packages/preact/__tests__/createFormValidityObserver.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});
});
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/preact/createFormValidityObserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
21 changes: 21 additions & 0 deletions packages/react/__tests__/createFormValidityObserver.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});
});
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react/createFormValidityObserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
21 changes: 21 additions & 0 deletions packages/solid/__tests__/createFormValidityObserver.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});
});
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/solid/createFormValidityObserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
21 changes: 21 additions & 0 deletions packages/svelte/__tests__/createFormValidityObserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});
});
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/createFormValidityObserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
21 changes: 21 additions & 0 deletions packages/vue/__tests__/createFormValidityObserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});
});
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/vue/createFormValidityObserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 7e2f30b

Please sign in to comment.