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

Support Rendering Error Messages to the DOM by Default #6

Merged
merged 11 commits into from
Apr 21, 2024
Merged

Conversation

ITenthusiasm
Copy link
Member

Although this feature is rather small (our 3kb bundle size didn't even change), it opens up some pretty significant doors:

1) Better Support for Using a Framework's Render Functions

There are frameworks like Lit and Preact which expect to always have control of the content of the DOM node to which they render entities. With this renderByDefault option, we can guarantee that for these frameworks, the renderer will always be used. So there should never be a time where the renderer suddenly stops working because the FormValidityObserver used the browser's default error message to directly modify an element's textContent (instead of relying on the framework's render function).

2) Support Managing UI Errors Via State

This is mainly a feature for React/Preact devs. It may not be that significant for devs using other frameworks.

There are developers who earnestly desire to keep their error messages in some kind of stateful object. They now have this option available if they "render" errors to a stateful object instead of rendering errors to the DOM directly. Then, they can use the stateful object to render error messages to the DOM instead. This isn't our preference or our recommendation. But if we can give people what they want with ease, then it's worth supporting. (More importantly, although we favor stateless forms in React, it's understandable why some might prefer the error messages to be stateful -- primarily to avoid having to think about things like useMemo or memo).

Note: The Solid Testing Library update seemed to break our
tests without a clear reason. And updating Vue and Preact
seemed difficult without handling the Solidjs issue. So...
This is where we area.
For some reason this one works, but not Vue's.
Although this feature is rather small, it opens up some
pretty significant doors:

1. There are frameworks like `Lit` and `Preact` which expect
   to always have control of the content of the DOM node to
   which they render. With this `renderByDefault` option, we
   can guarantee that in these situations, the renderer
   will ALWAYS be used. So there should never be a time where
   the renderer suddenly stops working because the
   `FormValidityObserver` used the browser's default error
   message and directly modified an element's `textContent`.
2. There are developers who earnestly desire to keep their
   error messages in some kind of stateful object. They
   now have this option available if they "render" errors
   to their stateful object instead of rendering errors
   to the DOM directly. Then, they can use the stateful
   object to render error messages to the DOM instead. This
   isn't our preference or our recommendation. But if we can
   give people what they want with ease, then it's worth
   supporting. (More importantly, although we favor stateless
   forms in React, it's understandable why some might prefer
   the error messages to be _stateful_ -- primarily to avoid
   having to think about things like `useMemo` or `React.memo`.

We're pretty happy that the types seem to be working fine, and
our new changes are backwards compatible. However, it is somewhat...
astounding how much TS effort was required compared to JS effort here
(including when it came to tests). We've come to the point that
@ThePrimeagen talked about where we're "writing code" in TypeScript.
Not willing to lose the DX from the IntelliSense at this moment, though.
Technically, the `renderByDefault` feature was already
working in the framework integration packages. But this
change makes sure that _TypeScript_ knows that as well.
No one wants their type checker yelling at them for
invalid reasons.

NOTE: This commit ALSO fixes an uncaught bug where
`useFormValidityObserver` wouldn't create a new instance
of the `FormValidityObserver` when the `defaultErrors`
option changed between state updates.
Note: We technically have not yet updated the types for
the JS framework integrations, so we aren't 100% sure that
our docs in that area are correct yet. But we're pretty
confident that the documentation should be correct.

After reviewing these docs on GitHub and verifying that we
didn't break any links, we'll follow up with the integration
TypeScript changes (and any docs updates that should be
done as a result).
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.
This will especially be helpful for React developers who
don't want to think about using `useMemo`/`React.memo`,
though hopefully `React Forget` will eradicate those
concerns sometime soon.
@ITenthusiasm ITenthusiasm merged commit cef04ef into main Apr 21, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant