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

feat(svelte-store): Svelte adapter #31

Merged
merged 15 commits into from
Jun 20, 2024
Merged

Conversation

david-roeger
Copy link
Contributor

This PR:

  • Adds initial implementation of the Svelte adapter
  • Adds initial tests for the Solid adapter

Regarding the questions in the issue:

  1. Packaging - I switched from tsub to svelte-package the official svelte way of well building packages. The build output from tsub threw errors when I tried to use it.
  2. Testing - I used testing-library svelte version. (The test for counting the rerender is a bit ugly because of onMount isn't called when rendering components  testing-library/svelte-testing-library#222)
  3. Reactivity interop - since svelte offers native store support this is quite nice. For the Api surface I tried to stay as close to the other packages.

Note: I just copy and pasted the shallow function - it might be worth refactoring that into the core package?

Closes #4

@david-roeger
Copy link
Contributor Author

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.

@david-roeger david-roeger mentioned this pull request Nov 5, 2023
@crutchcorn
Copy link
Member

crutchcorn commented Dec 13, 2023

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!

@LukeHagar
Copy link

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.

@david-roeger
Copy link
Contributor Author

david-roeger commented Dec 14, 2023

@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:
See david-roeger#1
Or in the repl

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?

@crutchcorn
Copy link
Member

crutchcorn commented Dec 16, 2023

Let's hold off for Svelte 5 - apologies for sending us all down this goose chase until then 😅😅

@david-roeger
Copy link
Contributor Author

Hey there,
with Svelte 5 now being in Release Candidate Stage I would be like to continue working on the adapter :)

@LukeHagar
Copy link

Sounds great to me!

@david-roeger
Copy link
Contributor Author

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

@crutchcorn
Copy link
Member

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

@wobsoriano
Copy link
Contributor

@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 theshallow function from the vanilla package so that it can be imported by the Vue, React, and Svelte packages.

@lachlancollins lachlancollins requested review from lachlancollins and removed request for LukeHagar June 20, 2024 01:15
Copy link

nx-cloud bot commented Jun 20, 2024

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

Copy link
Member

@lachlancollins lachlancollins left a 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.

@lachlancollins lachlancollins changed the title feat: Svelte adapter feat(svelte-store): Svelte adapter Jun 20, 2024
Copy link
Member

@crutchcorn crutchcorn left a 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!

@crutchcorn crutchcorn merged commit 71d8f65 into TanStack:main Jun 20, 2024
2 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.

Svelte Adapter
5 participants