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

Add solid adapter #6

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Apr 7, 2025

This sets up the solid-pacer package, and examples, and docs

Copy link

changeset-bot bot commented Apr 7, 2025

⚠️ No Changeset found

Latest commit: 18e1d53

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@birkskyum
Copy link
Member Author

birkskyum commented Apr 7, 2025

@KevinVandy , can you see what the problem is for eslint here that make the test suite fail?

@KevinVandy
Copy link
Member

Great PR. Don't know if you saw in this repos history, but I started out with a Solid adapter before I temporarily removed it a couple weeks ago to speed up the initial development. 8debd03

Your contribution is going to speed up the process a lot

@KevinVandy
Copy link
Member

All package.json's need to have a unique package name in the monorepo, or else pnpm i and pnpm build will probably fail. The solid examples still seem to have the react examples' names.

TFn extends (...args: Array<any>) => any,
TArgs extends Parameters<TFn>,
>(fn: TFn, options: AsyncDebouncerOptions) {
const [asyncDebouncer] = createSignal(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

back when I was experimenting with making my own solid adapter, I think I had discovered that most of the code in the solid adapter would be unnecessary. Like it could be as simple as createAsyncDebouncer = () => new AsyncDebouncer(options)`

Copy link

pkg-pr-new bot commented Apr 7, 2025

More templates

npm i https://pkg.pr.new/TanStack/pacer/@tanstack/pacer@6
npm i https://pkg.pr.new/TanStack/pacer/@tanstack/react-pacer@6
npm i https://pkg.pr.new/TanStack/pacer/@tanstack/solid-pacer@6

commit: 18e1d53

@birkskyum birkskyum marked this pull request as ready for review April 7, 2025 22:28
@birkskyum
Copy link
Member Author

birkskyum commented Apr 7, 2025

@KevinVandy there is something working here - not all, but quite a lot.

If we merge this, there will be docs, and we can easily browse the examples and fix the remainder.

@birkskyum birkskyum requested a review from KevinVandy April 7, 2025 23:07
@birkskyum
Copy link
Member Author

I've removed the dependency arrays from memo/effect - maybe the Callback versions aren't necessary at all for solid, since the useCallback isn't there.

@KevinVandy
Copy link
Member

I'll check out the PR and clean up som stuff. I'd like it to be mostly correct / on the right path before merging, or at least before releasing. We can merge and not release, but might as well work in this branch for a bit.

@birkskyum
Copy link
Member Author

Sounds good - I imagine there are quite a few lambda wrappers that can be removed.

@KevinVandy
Copy link
Member

KevinVandy commented Apr 8, 2025

I added 1 commit in the solid throttler area where I renamed and refactored some things to go in the correct direction. I won't be able to work on this again till tomorrow if you or someone wants to pick up the work again.

  • simplify the useDebouncer and similar (createDebouncer) to just return a new instance directly
  • "create" instead of use
  • "signal" instead of state
  • get rid of callbacks
  • options in examples act as a constructor. To change options in a solid app, you must use the setOptions API in an effect or event.
  • Morre changes in examples are needed like reading values from a queuer/debouncer, etc. in a render will need to use the onUpdate, or onGetNextItem callbacks to actually pull live values.

@birkskyum
Copy link
Member Author

birkskyum commented Apr 8, 2025

i opted for keeping the api surface intentionally, since it makes the docs easier to maintain, and they redirect correctly when you switch framework there - the use keyword is popular amongst all of react, vue and svelte 5 and solid, so it's nice to have solid just be the same as the more adopted frameworks, to avoid unnecessary migration steps for anyone giving solid a shot. It's also how things are done for solid in tanstack router (useParams, useSearch, useRouter ..) and query (useQuery, useMutation ..), store (useStore) and the "official" solid router (useNavigate, useMatch ..)

@KevinVandy
Copy link
Member

The create vs use naming convention is interesting. In TanStack Table, Virtual, and Form, we've used "create" for both solid and svelte. I didn't realize that Query and router were using "use"

@birkskyum
Copy link
Member Author

birkskyum commented Apr 8, 2025

Svelte 4 used create - svelte 5 is on use (https://userunes.com/), so it's purely for historical reasons that create is used for svelte, and it might even be due for an update to use when the svelte5 rewrite lands in query.

@KevinVandy
Copy link
Member

@birkskyum I find myself mostly agreeing with Ryan's naming convention, but it's not a hill I'd die on. I respect your opinion here with your background for sure.

Consider this though (potentially). React might be the only framework adapter for pacer that even needs a useDebouncer hook.

Maybe for all other frameworks, we just direct devs to use const debouncer = new Debouncer(myFunction, {...options})

But then, we can also provide "hooks" like createDebouncedSignal, which is just a debouncer wrapper around createSignal from Solid. In that case, I think that naming convention makes most sense still. The name perfectly describes how it is a wrapper.

@birkskyum
Copy link
Member Author

birkskyum commented Apr 8, 2025

Fair enough, I've update the api and the docs to create. Now there are 404s all over the place on the docs as a result when changing framework. I'd appreciate those in favor of having different api's for the frameworks (thus introducing that problem) will help resolving that

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.

2 participants