Skip to content

Latest commit

 

History

History
50 lines (36 loc) · 9.77 KB

TODO.md

File metadata and controls

50 lines (36 loc) · 9.77 KB

TODO

Documentation

  • Add more detailed examples of how to use Zod with the defaultErrors.validate option.
  • Figure out a Logo for the enthusiastic-js Organization and maybe the Form Observer package?
  • 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.
  • We currently have StackBlitz examples for the FormValidityObserver. Would examples for the FormObserver or the FormStorageObserver also be helpful?

Repository

  • PRIORITY Consider automating the process of publishing NPM Packages (with provenance) using GitHub Releases.
    • Seriously. This seems super easy. You should definitely do/try this the next time you release a new package version.
    • For the build-packages.js script, you might need to conditionally copy GitHub CI's .npmrc file to all of the workspace folders. This logic would be added to the addNPMFilesTo() function. (The condition is that this should only happen on CI. You can check GitHub CI's process.env.CI environment variable for this.) Then you can try npm publish --workspaces --provenance --access=public (after running the build script). But if you're unsure about how reliable the --workspaces option is, perhaps you can try looping through the folders on CI instead? (Let's hope that --workspaces works just fine...)
  • Consider adding some badges to our Repository's homepage (e.g., Test Coverage).

FormValidityObserver

  • Maybe we should have a "potential pitfalls" or "pro tips" section?
    • 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? 🤔
  • 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.
  • 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.
  • 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.)

FormValidityObserver Optimizations

  • 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...
  • 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 captured invalid event handler to make sure error messages are properly updated if needed.)
    • 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.
  • 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?)

FormValidityObserver Potential Future Ideas/Features

  • 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.)
  • 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
  • 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.)
    • 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.

TypeScript

  • Critical: Figure out what to do about the broken, overloaded generic constructor types for the FormObserver (and its children) -- specifically surrounding generic event types. This seems to be a bug in TypeScript, though it might be intentional since generic constructors are not allowed in .ts files at all... We don't want to rewrite our code again, and it seems a little difficult to create separate .d.ts files for the generic constructors (if our plan is to cast all of the default exports of classes to the constructor interfaces). So... We'll wait and see if the TypeScript team will update anything for now. See microsoft/TypeScript#55919 and microsoft/TypeScript#40451. At least all of the other types in our project are good, though! :D And those are the more important types! Plus, if people are using the Framework Integrations, they'll have all the types that they need anyway (at least for form field validation). So... it should be safe to release our package right now.

Testing

  • Figure out why @solidjs/testing-library isn't able to render raw elements correctly for testing.
  • It's weird that beforeEach(vi.restoreAllMocks) causes an error in TS. Maybe that ought to be a GitHub issue?
  • Temporarily, we have to change userEvent to a named import because of testing-library/user-event#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.)