feat(svelte-form): Add Svelte adapter #1247
Conversation
Absolutely untested, written without running it or trying it out in any way. Just a very rough start written off the top of my head with the help of various bits of docs.
- one Field component with a module script with createField in it - createForm file - bunch of types that pass along generics, closely modeled after the other adapters - tests
|
View your CI Pipeline Execution ↗ for commit f01a213.
☁️ Nx Cloud last updated this comment at |
|
Let me know if you want any help with this 👍 my pr went stale since I was in need of someone to pair/review with but I am still very much interested in landing this |
| ], | ||
| "dependencies": { | ||
| "@tanstack/form-core": "workspace:*", | ||
| "@tanstack/svelte-store": "^0.7.0" |
There was a problem hiding this comment.
this should come from workspace:* I think
There was a problem hiding this comment.
This isn't part of the monorepo, so like this is the correct way
| } from '@tanstack/form-core' | ||
| import { useStore } from '@tanstack/svelte-store' | ||
| import { onMount, type Snippet } from 'svelte' | ||
| import Field from './Field.svelte' |
There was a problem hiding this comment.
this is really, really odd. importing the file we're in, from the file we're in
i'm surprised this did something. even if this somehow works, we really shouldn't be doing it imo
this is why createField and Field were originally separate
There was a problem hiding this comment.
Why shouldn't we? This is valid JavaScript, you can self-import in ESM, it's part of the spec. This is just a different way of doing what the other adapters are doing, where they have Field and createField in the same file.
There was a problem hiding this comment.
its basically a hacky workaround for the fact there's a circular dependency here (createField needs Field and vice versa), and in svelte the component is the module as a whole
ideally, we'd figure it out properly. though im not sure how off the top of my head
There was a problem hiding this comment.
Are you running into a problem with the circular ref? In general it's ok due to the way js imports work (e.g., you can import yourself), but ideally something we resolve.
There was a problem hiding this comment.
im fully aware that this "works", its just not great for code quality/readability and is inconsistent with the rest of the codebase
also its a clear workaround for the circular dependency. you did this because you couldn't define Field in the file itself (since it is the file), whereas in all other integrations we would just have it in a named export
i disagree with doing it this way but will leave it up to other reviewers to decide
There was a problem hiding this comment.
You're not gonna get a "clean" solution because there's a cyclic dependency here. So either you have a cyclic import or you have a self-import - I prefer the latter due to colocation.
| extendedApi.createField = (props) => | ||
| createField(() => { | ||
| return { ...props(), form: api } | ||
| }) as never |
There was a problem hiding this comment.
how come you weren't able to type this properly? (i.e. without as never)
There was a problem hiding this comment.
added a comment; TS throws an error here that type instantiation is excessively deep (same as for Solid FWIW)
|
Is this ready for testing it in a project? |
And if it is, would love to know if there is a nightly build :), Otherwise what is left / where we can help. |
I see an error in types that has an @ts/error with no actual error, and as someone else pointed that import failing. I think the reviews need to approve and best guess they will once those two failing tests pass from the fixes I mentioned... |
| id="form" | ||
| onsubmit={(e) => { | ||
| e.preventDefault() | ||
| e.stopPropagation() |
There was a problem hiding this comment.
nit: we probably don't need to stop the propagation here. doesn't make much difference in these examples but would be good to avoid people thinking this is necessary
| const target = e.target as HTMLInputElement | ||
| subField.handleChange(target.value) | ||
| }} | ||
| /> |
There was a problem hiding this comment.
it may be me missing something, but html doesn't use self-closing tags anymore (they're ignored). stuff like this should be a void tag (<input>)
|
|
||
| import rootConfig from '../../eslint.config.js' | ||
|
|
||
| export default [...rootConfig] |
There was a problem hiding this comment.
do you think there's opportunity here to enable svelte rules for the svelte files in here?
not in this PR, but we could open an issue to do it separately if you think that makes sense
There was a problem hiding this comment.
honestly I don't care, Eslint isn't of much use in this repo in my opinion, so I'd rather just keep it as is
There was a problem hiding this comment.
to be clear, we were always going to keep it as it is in this pr. i was asking in case its worth us opening an issue to do it in a separate PR
not in this PR, but we could open an issue to do it separately
it may make sense for linting svelte sources since we'll have a few after this PR
|
some of the CI failure seems unrelated, some this all looks good other than the failing CI though 👍 i left a few very minor comments but nothing much there's an unfortunate amount of |
|
i tried this out locally and dug around the types a bit etc, all seems pretty good maybe we can get some kind of nightly build sorted soon |
This implements a Svelte adapter for the form package. API-wise it's somewhere inbetween Solid and React (
createFormis used like Solid's, getting the field state from within a snippet is more like React). Added basic tests and two examples.Superseeds #1107