Skip to content

Commit e0646f3

Browse files
committed
chore: Update/Cleanup TODOs List (Important)
There were some TODO items that are no longer relevant or that have already been satisfied. So we've updated this file accordingly to reduce clutter/noise. NOTE: There were some conclusions that we drew in this TODOs file that didn't make their way into our `DEVELOPMENT_NOTES.md` file. (Maybe they belong there?) You should check back to this commit in circumstances where you perhaps feel like you need more context. Or perhaps we should simply update our `DEVELOPMENT_NOTES` where appropriate? Below are some important/useful notes about some of the TODOs that we deleted. (TODOs that were deleted for more obvious reasons do not have explanations.) --------------------------------------------------------------------------------- 1) The TODO about not supporting nested form structure was misleading. People can use “nested forms” as long as they provide accurate names for the form fields submitted to the server. Other form validation libraries also use “parent.path.to.child” names for fields. 2) We’re going to (perhaps dangerously) assume that people know how to create semantic radio buttons. We have StackBlitz examples that show how to do this, and we link to documentation that shows how to do this as well. If it ends up being a problem that’s frequently brought up by React developers, then we can address the issue in our documentation. But it’s (presumably) highly unlikely that React devs are going to create radio buttons that are intended to belong to the same group but that have different `name`s. And our docs already explain that all validated fields _must_ have a valid `name`. 3) It doesn’t seem like we need to modify the `FormField` type. As mentioned in our TODOs, we already have `ValidatableField` for the `FormValidityObserver`. As far as the base `FormObserver` is concerned, none of its event handlers seem to have any problem using an `instance` check to narrow `event.target` down to a specific `HTMLElement` (including Web Components). Type narrowing will be needed for people who want to take Web Components into account in a specific way anyway. If they have any additional assumptions, they’ll need to create a separate type (just like we did for the `FormValidityObserver`). 4) As previously mentioned, we’re not going to use Vitest’s approach for testing TypeScript types. If we want an approach that would work for any tool, and we want an approach that won’t accidentally impact code coverage, then we should just stick to what we have for now. The `lint` script will check these types for us. So in a sense, they “run” the “TS type tests” in our test files.
1 parent 969beed commit e0646f3

File tree

1 file changed

+7
-17
lines changed

1 file changed

+7
-17
lines changed

TODO.md

+7-17
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55
- [ ] Add more detailed examples of how to use `Zod` with the `defaultErrors.validate` option.
66
- [ ] Figure out a Logo for the `enthusiastic-js` Organization and maybe the Form Observer package?
77
- [ ] In the interest of time, we're probably going to have to do the bare minimum when it comes to the documentation. Make the API clear, give some helpful examples, etc. After we've release the first draft of the project, we can start thinking about how to "perfect" the docs. But for now, don't get too paranoid about the wording.
8-
- [ ] Adding demos somewhere in this repo (or in something like a CodeSandbox) would likely be helpful for developers. **Edit**: We now have examples for the `FormValidityObserver`. Would examples for the `FormObserver` or the `FormStorageObserver` also be helpful?
8+
- [ ] We currently have StackBlitz examples for the `FormValidityObserver`. Would examples for the `FormObserver` or the `FormStorageObserver` also be helpful?
99

1010
## Repository
1111

12-
- [ ] Consider automating the process of publishing NPM Packages (with [provenance](https://docs.npmjs.com/generating-provenance-statements)) using GitHub Releases.
12+
- [ ] **PRIORITY** Consider automating the process of publishing NPM Packages (with [provenance](https://docs.npmjs.com/generating-provenance-statements)) using GitHub Releases.
1313
- Seriously. This seems super easy. You should _definitely_ do/try this the next time you release a new package version.
1414
- https://docs.github.com/en/actions/publishing-packages/publishing-nodejs-packages
1515
- https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions
@@ -18,23 +18,15 @@
1818

1919
## `FormValidityObserver`
2020

21-
- [ ] Add docs at some point on why we _don't_ allow structuring form data as nested objects like `React Hook Form`, `Conform`, etc.
22-
1. Trying to support this use case adds additional overhead to the project without adding much benefit to our users.
23-
2. The form data submitted to the server is not going to have this unusual structure anyway if you're using progressive enhancement (unless you want to validate 2 different structures of form data on the server ... which is unnecessarily complicated). Therefore, it's better to just embrace what HTML provides out of the box. A more reliable way of grouping form data (e.g., address fields) is perhaps to use `names` in a way that allows for "scoping". For example, using `address_street`, `address_city`, and the like is quite reliable. Then, the server can determine scoping by `split`ting form data keys by underscores (when needed).
2421
- [ ] Maybe we should have a "potential pitfalls" or "pro tips" section?
25-
- [ ] For example, anyone who extends the `FormObserver` should probably use arrow functions for event handlers if they ever want to use `this` ... It will make life much easier.
26-
- [ ] In a similar vein, should we add a warning that radio buttons must be semantically correct in order for them to participate in field validation? This really shouldn't be an issue; but since this isn't enforced by React (due to state), some people may need to know this. This would basically just mean that radios belonging to the same group would need to share a common `name`.
27-
- [ ] Should the `ValidationErrors` interface reference the classic form field properties, or the `ValidityState` properties? Using the regular form field properties is great if you're using regular HTML Elements. But it might be a little less intuitive for those using Custom Elements. (There are also downsides to shifting our approach. So this would require some thinking.) **EDIT**: This probably isn't a concern. However, for frameworks that handle attributes in an unorthodox way like `React`, we might have to think of a solution that takes care of _both_ regular fields _and_ Custom Elements. Regular JS frameworks like Svelte, Vue, and Solid will not have this problem.
22+
- [ ] For example, anyone who extends the `FormObserver` should probably use arrow functions for event handlers if they ever want to use `this` ... It will make life much easier. Then again, don't developers who are trying to use `this` already know that? 🤔
23+
- [ ] Should the `ValidationErrors` interface reference the classic form field attributes, or the `ValidityState` properties? Using the regular form field attributes is great if you're using regular HTML Elements. But it might be a little less intuitive for those using Custom Elements. (There are also downsides to shifting our approach. So this would require some thinking.) **EDIT**: This probably isn't a concern. However, for frameworks that handle attributes in an unorthodox way like `React`, we might have to think of a solution that takes care of _both_ regular fields _and_ Custom Elements. Regular JS frameworks like Svelte, Vue, and Solid will not have this problem.
2824
- [ ] The `aria-describedby` attribute technically supports multiple IDs at one time. Should we add a `data-describedby` (or `data-errormessage`) attribute for cases like these? We would need to enforce that the `data-errormessage` value is a substring of the `aria-describedby` value in this case (so that the error message is still _accessible_). However, it's hard to say how realistic this scenario would be. Anyone attempting to do this would also have to deal with the fact that the various descriptions would get joined together into a "single unit"... So they'd have to be mindful of the order of their `aria-describedby` ids anyway. This might be worth tackling, but it doesn't seem urgent; so we're delaying it.
29-
- [ ] Up to this point, we've been defining the `FormField` type as any `HTMLElement` that will _naturally_ appear in a [form's list of elements](https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/elements). However, this definition isn't fully accurate anymore given that we're now trying to be cognizant of Web Components. We need to figure out a new definition that works. There are several possible approaches to this problem... Perhaps we could support a custom `interface` that extends `HTMLElement` and supports/exposes all necessary properties/methods -- including those from `ElementInternals`. But how simple/complex is that? Is this something that we should delegate to userland? Should we just accept any `HTMLElement` and let the users narrow their types from there? We need to think more on this.
30-
- **Edit**: We created `ValidatableField` to handle this use case for the `FormValidityObserver`. Maybe we just need a general "FormControl" type? Only the `FormValidityObserver` needs the properties related to field validation, but the parent `FormObserver` still needs access to the `form` property... Maybe that could replace `FormField`? Maybe?
3125
- [ ] It's a little bothersome to us that in the `validateFields()` method, `getErrorOwningControl` technically has to be called twice when scrolling an invalid field into view. It's not the end of the world, but it doesn't feel like a clean solution either. This might be another reason to migrate towards checking `field.validity.valid` instead of `field.getAttribute("aria-invalid") === String(true)`. (This would require passing `errorElement.innerText`/`errorElement.textContent` to `setCustomValidity` whenever we use the `renderer` function to render error messages to the DOM.)
3226

3327
### `FormValidityObserver` Optimizations
3428

35-
- [ ] Would it make sense to make the `FormValidityObserver` its own thing? It may not need to extend the `FormObserver` at all since it only supports watching 1 form at a time...
36-
- [ ] Would it be helpful to have an `optimize` option for `FormValidityObserver` where the developer _promises_ to `configure` all fields that they want validated (and `deconfigure` anything that shouldn't be validated later), so that we can loop over all of the configured field `name`s instead of looping over `form.elements` when `validateFields` is called without an argument?
37-
- **Edit**: We probably won't be doing this because it seems to add significant complexity without much meaningful benefit. This is something that can be done in userland if needed. (And the actual performance gains that users will achieve with this are likely small.) Pushing this off to userland brings a little bit of complexity to the users who would like this functionality, but _most_ of the complexity that would be involved is something that the `FormValidityObserver` cannot simplify for our users anyway; React Hook Form's example of their [`unregister`](https://react-hook-form.com/docs/useform/unregister) function proves this. At best, we could store the configured fields for our users; but our users would still need to write their own logic for "unconfiguring" form fields responsively (just like with React Hook Form). And since users can easily store (and loop over) the configured fields in a `Map` (or whichever structure they prefer), it doesn't seem practical for the `FormValidityObserver` to take responsibility for storing that data.
29+
- [ ] Would it make sense to make the `FormValidityObserver` its own class? It may not need to extend the `FormObserver` at all since it only supports watching 1 form at a time...
3830
- [ ] Is there a way that we could call `form.checkValidity()` if **none** of the fields have a configured custom `validate` function? Would that be a meaningful performance boost -- if any? (If we did that, we might also need to add a `capture`d `invalid` event handler to make sure error messages are properly updated if needed.)
3931
- **Note**: It may be sufficient/appropriate to delegate this to user land. Perhaps we could simply add documentation saying, "For a performance boost, if you don't have any custom `validate` functions, just use `form.checkValidity()`" or something like that. (This, again, is _assuming_ that `form.checkValidity()` yields a significant performance boost over `observer.validateFields()`. We need to test that.) If we go with this approach, we'd probably still need to register `invalid` event handlers. So we need to think about how we'd go about that if we want to go that route.
4032
- [ ] Are there any ways that we can optimize updating the DOM? (For instance, not touching `element.textContent` if the error message didn't change. Is there even a significant performance benefit in doing that?)
@@ -43,9 +35,9 @@
4335

4436
- [ ] Provide a way for users to specify the value of `aria-invalid` (e.g., `spelling`). Maybe we could do this through the `ValidationErrors` configuration? (**Note: This idea might not even be significant or truly needed.**)
4537
- [ ] 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)
46-
- [ ] 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...
4738
- [ ] 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?
4839
- [ ] 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.)
40+
- Note: For what it's worth, as long as developers aren't using custom validation functions, `field.checkValidity()` and `form.checkValidity()` are perfectly fine to use on their own. We don't need a `silent` option for that use case. The `silent` option would only be relevant for developers who want silent validation _and_ who use custom validation functions. Otherwise, the browser's methods are enough.
4941

5042
## TypeScript
5143

@@ -55,6 +47,4 @@
5547

5648
- [ ] Figure out why `@solidjs/testing-library` isn't able to render raw elements correctly for testing.
5749
- [ ] It's weird that `beforeEach(vi.restoreAllMocks)` causes an error in TS. Maybe that ought to be a GitHub issue?
58-
- [ ] Temporarily, we have to change `userEvent` to a named import because of https://github.com/testing-library/user-event/issues/1146. Hopefully this gets fixed eventually. Maybe we can contribute something if we figure out this `NodeNext` headache on our own end.
59-
- [ ] Replace "Pure TypeScript Type Tests" with the equivalent in Vitest. It sounds like Vitest has something [designed for this](https://vitest.dev/guide/testing-types.html)?
60-
- **EDIT**: We're going to forego this for now. Because the [type-testing API](https://vitest.dev/api/expect-typeof.html) is specific to Vitest, it seems better to just keep what we have since it will be more flexible in the long run (between different testing tools). What we currently have still enables type checking (WITHOUT the risk of false positives on code coverage), and it keeps us from having to learn a tool-specific API for asserting valid types.
50+
- [ ] Temporarily, we have to change `userEvent` to a named import because of https://github.com/testing-library/user-event/issues/1146. Hopefully this gets fixed eventually. Maybe we can contribute something if we figure out this `NodeNext` headache on our own end. (I think that we did indeed figure it out.)

0 commit comments

Comments
 (0)