-
Notifications
You must be signed in to change notification settings - Fork 31
test: add contract test boilerplate to react universal #1074
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
92037b6 to
4052111
Compare
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
| </body> | ||
| </html> | ||
| ); | ||
| } |
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.
Next.js layout missing required children prop
Medium Severity
The layout.tsx component is missing the required children prop that Next.js App Router layouts must accept and render. Without this prop, any page content added to this app in the future won't be rendered, and the layout won't properly wrap its content. The function signature needs to accept { children } and render it inside the body.
|
|
||
| disconnect() { | ||
| this._ws?.close(); | ||
| } |
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.
Disconnect triggers reconnection instead of closing cleanly
Medium Severity
The disconnect() method calls this._ws?.close(), which triggers the onclose handler that unconditionally schedules a reconnection via setTimeout. When the React component unmounts and calls disconnect() during cleanup, this will paradoxically trigger a new connection attempt 1 second later. The class has no mechanism to distinguish intentional disconnection from unexpected connection loss.
Additional Locations (1)
| @@ -0,0 +1,64 @@ | |||
| // eslint-disable no-console | |||
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.
Same as https://github.com/launchdarkly/js-core/blob/main/packages/sdk/browser/contract-tests/entity/src/TestHarnessWebSocket.ts but with no client implementation yet.
| @@ -0,0 +1,42 @@ | |||
| #!/usr/bin/env node | |||
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.
| "packageManager": "yarn@3.4.1", | ||
| "scripts": { | ||
| "install-playwright-browsers": "playwright install --with-deps chromium", | ||
| "start:adapter": "yarn workspace browser-contract-test-adapter run start", |
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.
NOTE: I am using the adapter directly from the browser contract test... I think this is okay, probably should work on extracting this ws bridge to a common private module
| @@ -0,0 +1,24 @@ | |||
| 'use client'; | |||
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.
stubbed for now
4052111 to
3adff85
Compare
3adff85 to
9f20078
Compare
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| @@ -0,0 +1,64 @@ | |||
| // eslint-disable no-console | |||
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.
Invalid eslint directive syntax does nothing
Low Severity
The comment // eslint-disable no-console uses incorrect syntax. For file-wide disables, ESLint requires block comment syntax: /* eslint-disable no-console */. The single-line comment format only works with eslint-disable-next-line. While the directory is in .eslintrc.js ignorePatterns making this comment redundant anyway, the incorrect syntax is misleading.
Requirements
Related issues
sdk-1765
Describe the solution you've provided
Describe alternatives you've considered
I think this pattern of opening a ws bridge would be used in many other places (especially for client sdks). If this implementation goes well, then we should think about moving some of the tooling to the packages/tooling directory.
Additional context
Add any other context about the pull request here.
Note
Sets up a new React Universal SDK contract test app and integrates it into the repo.
packages/sdk/react-universal/contract-testsNext.js entity with WebSocket client (app/websocket.ts) and minimal page (app/layout.tsx) for communicating with the existing adapter; includes build/dev configs, Playwright headless browser helper, and start scriptpackage.json; updatespackages/sdk/react-universal/tsconfig.jsonto excludecontract-tests; adds ESLint ignore forreact-universal/contract-testshttp://localhost:8000for the harness URLPorts: adapter REST
8000, adapter WS8001, React entity8002. Capabilities response is currently stubbed inwebsocket.ts.Written by Cursor Bugbot for commit 9f20078. This will update automatically on new commits. Configure here.