-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(react-client): migrate from recoil to zustand #1870
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
base: main
Are you sure you want to change the base?
Conversation
I've just done the exact same thing before finding this PR https://github.com/jbeckerdm/chainlit/tree/zustand 😆 Looks great! This already allows custom frontends to update to React 19. |
- Migrate from recoil to zustand - Bump react to v19 # Conflicts: # libs/react-client/package.json # libs/react-client/src/state.ts # libs/react-client/src/types/index.ts # libs/react-client/src/useChatSession.ts
If someone finds time to take this one over, since I might not have enough time, I'd appreciate it. Or if yours gets merged, I'm fine with both, as long as we get progress on compatibility with the latest version 😄 |
I'm gladly taking this over. |
Wonderful 🙏, I've sent you an invite for my fork, but do whatever is easier for you. I don't really care, as long as we get some progress on this 😄 |
f6ac16d
to
c777d6c
Compare
Updated the source branch with my changes.
|
To make the tests run, I also replaced every usage of react-client's state in the chainlit frontend and copilot lib. |
All chainlit E2E tests run successfully. @willydouhard I'd like to ask you or other core chainlit maintainers for a first feedback on this, if possible. This enables upgrading the chainlit frontend as well as anyone using it to upgrade to React 19, and also allows for more flexible store structures. |
any update for this? :) |
Any update? Any help needed? |
@FlawaCLV @tegarmonster |
@jbeckerdm thanks for the update! I missed the news that they wanted to stop maintaining the project. I understand now why this PR is stalling. |
ah I see, thanks for the information |
@jbeckerdm
|
It's on the main page of the repo, 1st part of the readme |
3f4b422
to
7c70fa7
Compare
7c70fa7
to
cbc21ee
Compare
chatSettingsDefaultValue isn't properly populated yet, If anyone wants to look into that, feel free! |
I am 1 of 2 users who received maintainer access. My strong suit is backend though and I'm not sure if the other maintainer has frontend experience. Can some of you review this and document any breaking changes/workarounds? This seems like a great PR but given the creator has left we're all in a tough spot. I don't want to further delay the huge backlog of improvements any further (just pushed 25% of outstanding PRs) but this is a big change that deserves more community feedback especially since this isn't my area. |
@hayescode I'm essentially the new maintainer of this PR. |
But I encourage anyone to test this PR in their own way and review the code! I'll make sure to work through the feedback. |
@jbeckerdm Hi Jannis. I'm the other maintainer, and while React isn’t the primary framework of choice used by me or my company, I do have frontend experience and a background in building frameworks. I really appreciate your work, and in my opinion, the upgrade to React 19 looks good. However, switching from one state management library to another is a breaking change - especially since you’re modifying the shared Regarding performance: it’s great if switching to So, I’d like to propose the following plan:
Thanks again for your contribution! |
Hey, while performance boost is cool, the base idea of this PR was to enable upgrade to React 19. Latest version of It's really easy to dismiss performance improvement, if there isn't a bottleneck, but this one not it, at least not at its core. Basically, if chainlit doesn't change from |
@aleks3fs Thank you for the explanation! I didn't see the deprecation message anywhere until I opened their repo. In that case, I think we don't have many options left, so I'll review this PR. |
The aforementioned bug with chat_settings not loading has turned out to be a side effect of our custom frontend's connection logic, and is NOT related to this PR. |
Hey @asvishnyakov, Great to have you on board as maintainer. May I kindly request your attention to reviewing and, if appropriate, merging this change at your earliest convenience? Your assistance would be greatly appreciated. |
Just popping in to check if there’s any chance this PR could get a review soon. We’re kinda stuck on React 19 support without these changes, so merging this would really help out! Let me know if there’s anything you need from my end, or if there’s something holding things up. Thanks so much for keeping things moving — really appreciate all the work everyone’s putting in! |
CI needs to be updated to work with this |
@hayescode I'm working on that here and in few other PRs, let's wait until you return at Wednesday, I think it should be ready |
authConfig?: IAuthConfig; | ||
user?: IUser | null; | ||
|
||
threadHistory?: ThreadHistory; |
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.
While thread history is available only to authenticated users, I don't think it should be part of AuthState
. It should be a separate state instead, as including it violates SRP.
(state: T | ((state: T) => T), replace: true): void; | ||
} | ||
|
||
export const stateOrSetter = <T, K extends keyof T>( |
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.
|
||
import { stateOrSetter } from './utils'; | ||
|
||
// import { subscribeWithSelector } from 'zustand/middleware/subscribeWithSelector'; |
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.
Please remove a comment (did you forget something?)
"name": "@chainlit/react-client", | ||
"description": "Websocket client to connect to your chainlit app.", | ||
"version": "0.2.4", | ||
"version": "0.3.0", |
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.
Please don't change version, we do it only at release time
const chatSettingsValue = get().chatSettingsInputs.reduce( | ||
(form: { [key: string]: any }, input: any) => ( | ||
(form[input.id] = input.initial), form | ||
), | ||
{} | ||
); |
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.
Can we eliminate code duplication?
So actually, after careful review, here are my two biggest concerns:
@hayescode FYI |
Hello! I can see this is still open with a 2.8.0 milestone while 2.8.1 has been released. |
There is an issue with running
@chainlit/react-client
, with latest React 19 and issue seems to be connected to usage ofrecoil
.In this PR I've
recoil
withzustand
. I tried to combine logically connected parts of the state into stores.It'd be nice to have this in the latest version, but I haven't been able to test everything.