Support Rendering Error Messages to the DOM by Default #6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Although this feature is rather small (our
3kbbundle 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
LitandPreactwhich expect to always have control of the content of the DOM node to which they render entities. With thisrenderByDefaultoption, 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 theFormValidityObserverused the browser's default error message to directly modify an element'stextContent(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
useMemoormemo).