-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat(svelte-store): Svelte adapter #31
Conversation
For the setup of the whole package I mainly copied and pasted from the vue example and svelte-query. I hope the setup is fine. |
This looks great to me, but I'm not a Svelte-spert admittedly 😅 Tagging in @LukeHagar to review the Svelte code, as he was offering to help out with a Svelte adapter for Forms PS I'm so sorry I missed this PR - I just didn't see it at the time; no excuses. Sorry! |
I just wanted to follow up here, this looks like a good implementation, and I'm glad to see you using the existing Svelte store functions. That being said I think this will end up changing once runes come about. |
@LukeHagar That's what we talked about yesterday in the discord. I played a bit around with runes and that's what I got so far: But the tooling around svelte-5 is... well not there yet and I can't get svelte-package to output types for index.svelte.ts files or tests working @crutchcorn does it make sense to release the adapter in it's current form for svelte-4 or should we just wait until svelte-5 arrives? |
Let's hold off for Svelte 5 - apologies for sending us all down this goose chase until then 😅😅 |
Hey there, |
Sounds great to me! |
I finished the implementation today. I had to disable the code-coverage for tests because I ran into some svelte-5 releated issues (e.g. https://cloud.nx.app/runs/q2YUKafLGK) and I had to disable knip for the package because of this issues using vite-plugin-svelte conflict |
Hey all! Sorry I missed this. Asking some folks to review now via socials. I'm not super familiar with Svelte 5 yet. Let me also tag in @lachlancollins to see if we can fix up Knip and Nx in this PR |
@crutchcorn Not a Svelte pro, but I've been experimenting with runes (trying to convert the svelte-sonner package to runes), and I can say the implementation looks good here. The only suggestion I have is the possibility of exporting the |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit cfa4042. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
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.
Hi @david-roeger , I've made structural changes to the adapter to better match how we do it in other TanStack projects (e.g. https://github.com/TanStack/query/tree/main/packages/svelte-query) - this removes the dependency on @sveltejs/kit
(no more svelte-kit sync
).
@crutchcorn I've fixed the Knip issues, but there does seem to be a Vitest coverage issue with the v5 RC, I'm sure it'll be fixed sometime soon.
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.
This is awesome, thank you so much for your help here!
This PR:
Regarding the questions in the issue:
Note: I just copy and pasted the shallow function - it might be worth refactoring that into the core package?
Closes #4