Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Support Field Revalidation #9

Merged
merged 8 commits into from
May 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

## Priority

- [ ] Update `@form-observer/lit` to automatically use `renderByDefault` and Lit's `render` function.
- [ ] Release `0.9.0` after doing everything ABOVE this task.
- [ ] CONSIDER updating `@form-observer/lit` to automatically use `renderByDefault` and Lit's `render` function.
- [ ] Alternatively, consider updating our StackBlitz Lit example to have a renderer example and a default-renderer example.
- [ ] Add docs on how to reconcile error messages between server and client.
- [ ] **Consider** adding some kind of `revalidateOn` option for the `FormValidityObserver`. (For example, after a field has become invalid `onfocusout`, someone might want that field to be continuously validated `oninput` until it is corrected.)
- [ ] Consider doing this AFTER releasing `0.9.0` if you need to.

## Documentation

Expand Down Expand Up @@ -47,6 +49,7 @@
- [ ] Perhaps we should dispatch the `invalid` event when validation fails? Just like the browser does? If we're claiming to _enhance_ the browser's functionality, then we don't really want to _take away_ anything that the browser does. Instead, we want to _add_ to it (as effectively, powerfully, and minimalistically as possible). **Edit**: We won't be supporting this any time soon unless people explicitly request it. See our [Development Notes](./docs/extras/development-notes.md#why-doesnt-the-formvalidityobserver-dispatch-invalid-events-when-an-accessible-error-gets-displayed)
- [ ] Would it make sense to support default error messages for the various native browser constraints? For instance, oftentimes the same static (but custom) error message is used for `required` fields. Having configurable default error messages can help save people from duplicating code unnecessarily. This should be pretty easy to add if people want it...
- [ ] Currently, we _technically_ allow developers to do both _accessible_ error messaging and _native_ error messaging simultaneously. It isn't encouraged, but it might be helpful for developers transitioning from one mode to another. Even so, this could be a source of confusion. This already makes our code a little confusing sometimes (potentially) since we effectively determine whether or not a field uses accessible errors based on its `aria-describedby` attribute. If we had an option for the `FormValidityObserver` constructor that let the developer determine whether they were using accessible errors or native errors from the start, that could potentially be more useful/clear... It would at least be more helpful from a maintainability standpoint... well, potentially. Is it worth trying? Or not?
- [ ] Consider adding a `silent: boolean` option to the `validateField()` and `validateFields()` methods. Perhaps there are some people who want to be able to validate their field(s) manually without displaying the error message(s) to users? We don't know how common that use case is... But this feature would provide greater alignment with methods like `field.checkValidity()` and `form.checkValidity()`, which avoid generating noise for users. (Ideally, we'd want developers to be able to use enhanced versions of both `field.reportValidity()`/`form.reportValidity()` **_and_** `field.checkValidity()`/`form.checkValidity()` so that they don't "lose" any browser features.)

## TypeScript

Expand Down
97 changes: 97 additions & 0 deletions docs/extras/design-decisions.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,103 @@ This file is similar to a `Changelog`: It specifies the dates (in descending ord

It's possible that the need for this is already captured in the concept of `PR` (Pull Request) history. We will try to run with this approach _and_ the approach of PRs before coming to a final decision on what to use to accomplish this history-preserving goal.

## 2024-05-04

### Deprecate the `useFormValidityObserver` React Hook

Originally, we thought that it would be a good idea to memoize the `FormValidityObserver` for the React developers who would use our tool. That was the idea behind our `useFormValidityObserver` hook: It would enable developers to use the `FormValidityObserver` without having to think about memoization. It's a great idea! And when possible, I think that maintainers should take this route! But unfortunately, this approach is not always an option...

Consider a scenario where a developer uses the `useFormValidityObserver` hook with a complex configuration:

```tsx
import { useFormValidityObserver } from "@form-observer/react";

function MyForm() {
const { configure, ...rest } = useFormValidityObserver("focusout", {
revalidateOn: "input",
renderer(errorContainer, errorMessage) {
// Implementation ...
},
defaultErrors: {
// Default error message configuration ...
},
});
}
```

The result returned by `useFormValidityObserver` is memoized. However, the `useMemo` hook (which the function calls internally) uses the function's `type` argument and object options as its dependencies. If the `MyForm` component re-renders for any reason, the `"focusout"` and `"input"` strings will remain constant. But the `renderer` function and the `defaultErrors` object will be re-defined during re-renders, causing the `useMemo` hook to create a new `FormValidityObserver`. So those options need to be memoized.

```tsx
import { useMemo, useCallback } from "react";
import { useFormValidityObserver } from "@form-observer/react";

function MyForm() {
const { configure, ...rest } = useFormValidityObserver("focusout", {
revalidateOn: "input",
renderer: useCallback((errorContainer, errorMessage) => {
// Implementation
}, []),
defaultErrors: useMemo(
() => ({
// ... Default error message configuration
}),
[],
),
});
}
```

As you can see, this code becomes very ugly very quickly. Even worse, _the code becomes less performant_. Why? Well, because memoization comes with a cost. And the more that memoization is used, the more costs users will have to pay. Yes, memoization is helpful. Yes, under the right circumstances, memoization can bring performance benefits. But it should be used as little as possible and only where it's truly needed.

Here in our example, we're performing 3 memoizations for a single function call. (One for `defaultErrors`, one for `renderer`, and one for `useFormValidityObserver`'s internal call to `useMemo`.) That's not good. A better solution is to memoize the entire result once. But wait... Couldn't we just do that with `createFormValidityObserver`?

```tsx
import { useMemo } from "react";
import { createFormValidityObserver } from "@form-observer/react";

function MyForm() {
const { configure, ...rest } = useMemo(() => {
return createFormValidityObserver("focusout", {
revalidateOn: "input",
renderer(errorContainer, errorMessage) {
// Implementation
},
defaultErrors: {
// ... Default error message configuration
},
});
}, []);
}
```

In fact, this is _more performant_ than `useFormValidityObserver`! If we memoize the result from `useFormValidityObserver`, then we perform _2_ memoizations. (One for `useFormValidityObserver`'s internal call to `useMemo`, and one for _our own_ call to `useMemo` on the function's result). But if we simply memoize the result of `createFormValidityObserver` directly, then we only have to perform _1_ memoization.

What does all of this mean? Well... It means that by default, even in the most ideal scenario, `useFormValidityObserver` will never be more performant than calling `useMemo` on `createFormValidityObserver` directly. And in most cases, `useFormValidityObserver` will be _less_ performant. Moreover, many developers won't even be aware of the concerns that we've discussed so far; so they're much more likely to footgun themselves if they use the flashy `useFormValidityObserver` hook!

We wanted to find a way to protect developers from `useMemo`, but we couldn't. (And honestly, no developer who wants to be skilled with React can avoid learning about `useMemo` in this day and age -- not until React Forget stabilizes, at least.) This dilemma is just an unavoidable downside of how React decides to operate as a framework, and we sadly can't do anything to fix that. What we _can_ do is guide developers on how to use `createFormValidityObserver` in React easily and performantly. And that's exactly what we've decided to do instead: Add clearer documentation as a better alternative.

Thankfully, things really aren't that complicated. People just need to wrap `createFormValidityObserver` inside `useMemo` (when using functional components). That's it.

Sidenote: Although we're removing the `useFormValidityObserver` hook, we aren't removing React as a peer dependency [yet]. The reason for this is that we're trying to figure out how to prepare for [React 19's changes to ref callbacks](https://react.dev/blog/2024/04/25/react-19#cleanup-functions-for-refs) (for the sake of `autoObserve`), and we're debating using the React package to help us with this. For example, we could use the React package to tell us the React version being used. Then, we could return one kind of `autoObserve` function if the React version is less than 19, and return a different kind of `autoObserve` function if the React version is greater than or equal to 19. But maybe that's a bad idea? Maybe we should just maintain different versions of our `@form-observer/react` package? Then again, it might not be a bad idea either since it seems like `import { version } from "react"` is [permitted](https://github.com/facebook/react/blob/main/packages/react/index.js#L78) (at least as of today). We'll see...

## 2024-04-28

### Only Allow One (1) Event Type to Be Passed to the `FormValidityObserver` Constructor

Although this is a breaking change, it is one that will more easily allow us to support the `revalidateOn` feature that we mentioned on GitHub while also maintaining the parent-child relationship between the `FormObserver` and the `FormValidityObserver`. (We could technically choose to break that relationship, but the reusable `observe` and `unobserve` methods seem to be proving helpful right now.)

It technically isn't _that_ difficult to continue supporting an array of strings for the `types` argument of the `FormValidityObserver` constructor. But the (very small) additional effort for this support did not seem worth it.

Practically speaking, most (if not all) developers are likely going to choose only one event type anyway. And they'll have to choose one of the events that actually relate to user interaction (so that the form is only validated as users interact with it). The most common of these events are `input`, `focusout` (the bubbling version of the `blur` event), and `change`. But each of these events renders the other ones irrelevant.

For example, the last `input` event that's triggered for a form field will always be triggered _before_ `change` or `focusout`. Consequently, it doesn't make sense to add `change` _or_ `focusout` to the `types` argument of the `FormValidityObserver` constructor when `input` is supplied. The developer would only need to specify the `input` event itself.

Similarly, the `change` event is only fired a _subset_ of the times that the `focusout` event is fired. (There are technically exceptions to this rule, such as `<select>` and `<input type="checkbox">`. But practically speaking, this rule holds for all form controls -- including those.) So instead of specifying both events, a developer would only need to specify the `focusout` event. Finally, if a developer wants to respond to `change` events exclusively, then they will only specify _that_ event. Neither `focusout` nor `input` could be added in that scenario because it would cause the form controls to be validated too frequently.

In the end, whichever event type is chosen, it is only practical for the developer to specify 1 event. Technically speaking, developers could try to specify custom events. But those events would likely be emitted in _response_ to `input`/`focusout`/`change` events anyway. So everything ultimately comes down to those 3 events.

In conclusion, it will be simpler for us to only allow 1 event to be specified for the `types` argument moving forward. Since this is also what will happen naturally (and since enforcing this might protect some developers from unexpected footguns), this seems like a good change.

## 2023-08-20

### Deprecate the `FormValidityObserver.validateFields(names: string[])` Overload
Expand Down
Loading