-
Notifications
You must be signed in to change notification settings - Fork 1.7k
add polling transport option #5955
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
CodSpeed Performance ReportMerging #5955 will not alter performanceComparing Summary
|
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.
Greptile Overview
Greptile Summary
This PR adds configurable polling transport support as an alternative to WebSocket connections. The implementation allows users to set transport: "polling" or transport: "websocket" in their config, and this value flows through to both the backend Socket.IO server and frontend client.
Key Changes:
- Added
transportconfig field with Literal typing ("websocket"|"polling") - Modified Socket.IO server initialization to use
config.transportinstead of hardcoded["websocket"] - Updated frontend to read transport from
env.TRANSPORTinstead of hardcoded value - Added
TRANSPORTkey to env.json generation
Critical Issues:
- CORS Configuration Regression: The PR hardcodes
cors_allowed_origins=[]andcors_credentials=False, completely ignoring user'sconfig.cors_allowed_originssetting. This breaks existing apps with custom CORS requirements. Socket.IO polling requires CORS configuration for the initial HTTP handshake. - Transport Upgrade Blocked:
allow_upgrades=Falseprevents automatic upgrade from polling to WebSocket, which may not be the desired behavior for all use cases.
Confidence Score: 1/5
- This PR has critical regressions that will break existing production apps with custom CORS configurations
- The hardcoded CORS configuration (
cors_allowed_origins=[],cors_credentials=False) removes support for user-configured CORS settings that were previously respected. This is a breaking change that will prevent WebSocket/polling connections in production environments where CORS is properly configured. The feature implementation itself is sound, but the CORS regression must be fixed before merging. - Pay critical attention to
reflex/app.py- the CORS configuration must be restored to respectconfig.cors_allowed_origins
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| reflex/app.py | 1/5 | Critical CORS configuration regression - hardcoded empty array breaks user config; allow_upgrades=False may prevent websocket upgrade |
| reflex/config.py | 5/5 | Added transport config option with proper Literal typing - clean implementation |
| reflex/utils/build.py | 4/5 | Added TRANSPORT to env.json - minor style issue with hardcoded string literal |
| reflex/.templates/web/utils/state.js | 5/5 | Changed from hardcoded "websocket" to dynamic env.TRANSPORT - correct implementation |
Sequence Diagram
sequenceDiagram
participant User as User Config
participant Build as build.py
participant EnvJSON as env.json
participant Frontend as state.js
participant App as app.py
participant SocketIO as Socket.IO Server
User->>App: Set transport config
Note over User,App: Config: transport = "websocket" | "polling"
Build->>User: Read config.transport
Build->>EnvJSON: Write TRANSPORT value
Note over EnvJSON: env.json contains TRANSPORT key
Frontend->>EnvJSON: Read env.TRANSPORT
Frontend->>SocketIO: Connect with [env.TRANSPORT]
Note over Frontend,SocketIO: Previously hardcoded ["websocket"]
App->>SocketIO: Initialize AsyncServer
Note over App,SocketIO: transports=[config.transport]<br/>allow_upgrades=False<br/>CORS hardcoded to []
alt Websocket Transport
SocketIO-->>Frontend: WebSocket connection
else Polling Transport
SocketIO-->>Frontend: HTTP long-polling
Note over SocketIO,Frontend: Cannot upgrade to WebSocket
end
4 files reviewed, 3 comments
No description provided.