-
Notifications
You must be signed in to change notification settings - Fork 474
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
Add Svelte TypeScript support #1866
Conversation
|
||
return { | ||
body: html, | ||
head: [head, `<style data-vite-css>${css.code}</style>`], |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@punyflash Thanks so much for putting this together! Going to do my best to get this reviewed myself and by @pedroborges (he knows more about Svelte than I and has helped with this adapter in the past). @jakobbouchard would love your 👀 on this one too if you have time, no pressure though! @james-em thanks for reviewing already 👍 |
Ok, I'll try to keep it up to date, but hopefully it not get staled as well |
Any news about this? I would hate to see this stale again :( I have generated I have been using it for the past week without any issue. |
@jamesst20, as for now, you may use package |
Thanks, I agree it appears the official repository is not well maintained when we take a look at how many valid issues/prs are being issued and just left in the dark. It's a shame considering how great it could get over time with active development. However it seems @westacks/inertia-svelte wasn't forked from this repository which is a bummer for us as an organization as we can't tell how far behind/ahead it is from the official repository. It's likely only ahead but there is no way to tell. By the way @james-em is my organization account. I didn't realize I was signed on this account at the time. |
I didn't have any time to test it, but from going through the diffs, it seems to be fine! I wish I could've helped more, but I no longer work in web development, so I don't really have my environments setup anymore and not a lot of free time either. |
@punyflash Hey just did a bunch of testing on this PR, and things seem to work well! I also made a bunch of small formatting tweaks. Still waiting on @pedroborges to have a look at this PR, but if he doesn't have a chance within the next couple days I think I'll just merge it in. Thanks again for all your work on this! |
@jakobbouchard Hey I noticed that your original PR didn't use SvelteKit to compile types — was there any particular reason for that? |
import isEqual from 'lodash/isEqual' | ||
import { writable, type Writable } from 'svelte/store' | ||
|
||
interface InertiaFormProps<TForm extends Record<string, unknown>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can improve a little the typing here using the same types that React uses.
See here https://github.com/jamesst20/inertia/blob/main/packages/svelte5/src/lib/useForm.svelte.ts#L19
I also was able to get rid of all ts-ignore
recentlySuccessful: boolean | ||
processing: boolean | ||
setStore(data: InertiaFormProps<TForm>): void | ||
setStore(key: keyof InertiaFormProps<TForm>, value?: any): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe setStore
should not include the attributes or methods of the form. Should be typed to the data.
In my branch I did
setStore(data: TForm): void
setStore(key: keyof TForm, value?: FormDataConvertible): void
defaults(): this
defaults(fields: Partial<TForm>): this
defaults(field?: keyof TForm, value?: FormDataConvertible): this
return options.onStart(visit) | ||
} | ||
}, | ||
onProgress: (event: AxiosProgressEvent) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking internally, it appears event can be undefined too
It seems he wrote a an ESBuild script by hand https://github.com/inertiajs/inertia/pull/1614/files#diff-e1d807461eb96d0bbba189479a9a91e7fa45dad230985c34563619bcb6d19e01R1 instead of using Sveltekit which is the documented approach. When you run He probably did that to follow what was done in other adapters like react https://github.com/inertiajs/inertia/blob/master/packages/react/build.js Doing so is more likely to cause pain later. SvelteKit is recommending to bundle ESM instead of CJS |
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
@james-em Hey! Just merged this PR in, but before we tag the release would you mind submitting your recommended changes as a new PR (or multiple PRs if that makes more sense)? @punyflash Huge thanks for all your efforts here! 💪 |
Will do as quickly as possible! Thanks! |
@james-em Thanks buddy! |
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
Merge punyflash/inertia with some tweaks and fixes
This reverts commit 519272b.
Looking forward to this being released! ❣️ |
This PR adds complete TypeScript support for Svelte adapter. Been tested by me on multiple projects and contributors from @westacks/inertia-svelte. Works perfectly for Svelte 3 and 4.
TypeScript support based on #1614, however:
cjs
files, since Svelte compiler requires ESM modules anyway.Also includes fixes to SSR CSS rendering to prevent yanking on initial page load: #1761 (will not be needed in Svelte 5)