-
-
Notifications
You must be signed in to change notification settings - Fork 81
feat: add Preact adapter #254
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
Conversation
packages/preact-store/src/index.ts
Outdated
| @@ -0,0 +1,97 @@ | |||
| import { useSyncExternalStoreWithSelector } from 'use-sync-external-store/shim/with-selector.js' | |||
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.
it's ok to use uSES shim in preact?
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 think we would need to at least switch the import to import { useSyncExternalStore } from 'preact/compat';
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.
You're right! I totally missed this.
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.
fixed this. no longer has the react uSES, but rather preact/compat. But it does use a simpler version of the "with selector" functionality since the original preact/hooks version ships without the with-selector and server-side sync. Let me know if you find something off about the with-selector implementation
ref: https://github.com/preactjs/preact/blob/main/compat/src/hooks.js
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.
FWIW, preact/compat is side-effectful & brings in a bunch of other stuff alongside usage of any API. If there is a need for our impl of uSES (or other APIs) we may be willing to release a separate shim package or separate entry point that consumers can use instead.
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.
That is actually so real. It would be awesome if this can exist. I would love to look into it as well!
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.
@rschristian you are a legend - thank you for jumping in here! Let us know how we can assist, but confirmed that it will be helpful. @theVedanta and crew are porting Query, Form, Store, Router, and others to Preact
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've talked about it for years, might finally be time for pure preact/compat/utils or something (name TBD)
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.
that would be awesome man you're such a goat for this
…into preact-adapter
|
This is ready to review now! |
crutchcorn
left a comment
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'd love to see some tests using testing-library like we have for React before we merge:
https://github.com/TanStack/store/blob/main/packages/react-store/tests/index.test.tsx
This would give me (personally) better confidence in the useSyncExternalStore custom hook.
I'm on it 🫡 |
|
View your CI Pipeline Execution ↗ for commit 88337db
☁️ Nx Cloud last updated this comment at |
Wrote the tests, and left a comment on the one that specifically tests the |
|
I'm about to update this pr with generated docs |
Store works out of the box with pretty much the react adapter, only a few changes were needed. Feel free to suggest fixes or co-author!
Making a store preact-adapter should essentially open doors to making preact adapters for more Tanstack libraries, so any help is appreciated here.