-
Notifications
You must be signed in to change notification settings - Fork 45
Bundling with React 19 + React compiler & dependency upgrades #1735
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: dev
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This pull request upgrades Session Desktop to React 19 with React compiler support and updates several major dependencies (Electron Builder 26, Redux 5, Zod 4). The changes include Babel for bundling and Terser for minification, along with refactoring of test utilities, selectors, and type definitions to align with the new library versions.
Key Changes
- Upgraded React 18→19, Redux 4→5, Zod 3→4, Electron Builder to v26
- Integrated React compiler and updated build toolchain (Babel, Terser)
- Migrated test utilities from react-test-renderer to @testing-library/react
- Refactored selectors to be hook-independent for React compiler compatibility
- Fixed type definitions for better type safety with React 19
Reviewed changes
Copilot reviewed 166 out of 167 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Major dependency upgrades including React 19, Redux 5, Zod 4, Electron Builder 26, Babel, and Terser |
| ts/util/urlHistory.ts | Updated Zod enum usage from z.nativeEnum to z.enum for Zod 4 compatibility |
| ts/types/Util.ts | Added JSX type import for React 19 compatibility |
| ts/themes/globals.tsx | Improved type safety by splitting duration functions and fixing type coercion |
| ts/test/components/renderComponent.tsx | Migrated from react-test-renderer to @testing-library/react with new testing APIs |
| ts/test/components/SessionInput_test.tsx | Updated test to use new findAllByTagName API |
| ts/test/components/AvatarPlaceHolder_test.tsx | Updated tests to use DOM queries instead of React element tree inspection |
| ts/state/selectors/userGroups.ts | Exported getGroupById for use in other selectors |
| ts/state/selectors/selectedConversation.ts | Refactored hooks to use selector functions for React compiler compatibility |
| ts/state/selectors/proBackendData.ts | Major refactor adding data processing logic and mock feature flag support |
| ts/state/selectors/onions.ts | Exported selectors directly, removing intermediate hook wrappers |
| ts/state/selectors/messages.ts | Removed unnecessary useMemo calls and refactored for React compiler |
| ts/state/selectors/groups.ts | Exported selectLibGroupName for reuse |
| ts/state/reducer.ts | Added RootState type export |
| ts/state/ducks/types/releasedFeaturesReduxTypes.ts | Renamed hooks to *Memo pattern for clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function useAvatarBgColorInternal(pubkey: string) { | ||
| return useAvatarBgColor(pubkey); | ||
| } |
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 fine with me, but I assume this is not enough to make react compiler happy?
| function useAvatarBgColorInternal(pubkey: string) { | |
| return useAvatarBgColor(pubkey); | |
| } | |
| const useAvatarBgColorInternal = useAvatarBgColor |
| @@ -1,4 +1,5 @@ | |||
| import styled from 'styled-components'; | |||
| import type { JSX } from 'react'; | |||
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 needs to be here?
| const { docX, docY } = useMouse(contextMenuRef); | ||
| const contextMenuRef = useRef<HTMLDivElement | null>(null); | ||
| // FIXME: remove as cast | ||
| const { docX, docY } = useMouse(contextMenuRef as RefObject<Element>); |
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 this be removed?
| // NOTE: [react-compiler] this has to live here for the hook to be identified as static | ||
| function useConversationDetailsInternal() { | ||
| const selectedConversation = useSelectedConversationKey(); | ||
| const hasMessages = useSelectedHasMessages(); | ||
| const isGroupV2 = useSelectedIsGroupV2(); | ||
| const isInvitePending = useLibGroupInvitePending(selectedConversation); | ||
|
|
||
| const isPrivate = useSelectedIsPrivate(); | ||
| const isIncomingRequest = useIsIncomingRequest(selectedConversation); | ||
|
|
||
| return { | ||
| selectedConversation, | ||
| hasMessages, | ||
| isGroupV2, | ||
| isInvitePending, | ||
| isPrivate, | ||
| isIncomingRequest, | ||
| }; | ||
| } |
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.
All of those are a bit annoying, but hopefully we can find a way to make this work without the hassle at some point.
This returns a new object each times though, no need to wrap it into a useMemo supposedly, correct?
| const t4 = useTriggerPosition(r4); | ||
| const t5 = useTriggerPosition(r5); | ||
| const t6 = useTriggerPosition(r6); | ||
| // FIXME: remove as cast |
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 be fixed now?
| const ref = useRef<HTMLDivElement>(null); | ||
|
|
||
| const triggerPos = useTriggerPosition(ref); | ||
| // FIXME: remove this as cast |
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.
👀
| export function useShowDeleteGroupCb(conversationId?: string) { | ||
| // Note: useShowLeaveGroupCb and useShowDeleteGroupCb are dependent on each other | ||
| // so I kept them in the same file | ||
| // NOTE: [react-compiler] if we memoise this ourselves the compiler gets angry, but it will memoise it for us |
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.
👀
| export const SessionInboxView = () => { | ||
| if (!window.inboxStore) { | ||
| return null; | ||
| throw new Error('window.inboxStore is undefined in SessionInboxView'); |
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.
the unit tests are fine with 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.
Where did you move 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.
damn, I didn't think we'd had to write that much to get babel setup!
Upgrades:
Building:
Dependency tweaks: