-
Notifications
You must be signed in to change notification settings - Fork 55
NonReact mode enhancements #592
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
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @module("relay-runtime") | ||
| external commitMutation: (RescriptRelay.Environment.t, 'config) => unit = "commitMutation" | ||
|
|
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.
Preserve disposable from NonReact commitMutation
In the new NonReact mutation runtime, commitMutation is bound as returning unit instead of the disposable Relay emits (RescriptRelay_MutationNonReact.res lines 18-19). The React binding still returns Disposable.t, so when -non-react is enabled any generated mutation helpers now drop the disposable and callers can’t unsubscribe or cancel an in-flight mutation, leading to potential leaks or duplicate callbacks.
Useful? React with 👍 / 👎.
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.
@codex change the signature to return a disposable please.
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.
Summary
- Updated the NonReact
commitMutationbinding to return the RelayDisposable.t, preserving the ability for callers to cancel or unsubscribe from mutations. packages/rescript-relay/src/RescriptRelay_MutationNonReact.resL5-L52
Testing
- Not run (not requested)
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 PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ~fRef: | ||
| (fRef | ||
| |. [%e valFromGeneratedModule ["getFragmentRef"]]) | ||
| (fRef |. [%e valFromGeneratedModule ["getFragmentRef"]]) |
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.
Bug: Dead code: readInline never generated in NonReact mode
The readInline function is never generated for inline fragments when NonReact mode is enabled. The outer match on line 143-144 returns an empty list when !NonReactUtils.enabled is true (| true, _ -> []), so the code block at lines 145-163 only executes when !NonReactUtils.enabled is false. Inside that block, the inner if !NonReactUtils.enabled check on line 147 can never be true, making the RescriptRelay_FragmentNonReact.readInlineData_ branch dead code. Users in NonReact mode with inline directive fragments will not have access to readInline.
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.
Fix this.
| "waitForFragmentData" | ||
|
|
||
| let read = (~node, ~convertFragment: 'fragment => 'fragment, ~fRef) => | ||
| fRef->readFragment_(node)->convertFragment |
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.
Bug: Arguments reversed in NonReact fragment read functions
The read and readInlineData_ functions pass arguments to the external runtime functions in the wrong order. The external readFragment_ expects (node, fRef) but fRef->readFragment_(node) translates to readFragment_(fRef, node). Similarly, readInlineData expects (node, fRef) but the call passes (fRef, node). This would cause the Relay runtime to receive incorrect argument types, resulting in runtime errors when reading fragment data in non-React mode.
More enhancements to provide a way to run RescriptRelay cleanly without depending on React.
Note
Introduce a React-free mode and move all React-specific APIs to
RescriptRelayReact, updating PPX/codegen, runtime modules, tests, and peer deps.rescriptRelayMode: "Default" | "NonReact"and PPX flag-non-react.RescriptRelayintoRescriptRelayReact(Context Provider,useEnvironmentFromContext,useSubscribeToInvalidationState,loadQuery).RescriptRelayand reference new module across codebase.RescriptRelay_QueryNonReact(fetch,fetchPromised,retain,commitLocalPayload),RescriptRelay_MutationNonReact(commitMutation),RescriptRelay_FragmentNonReact(read,readInlineData,waitForFragmentData).NonReactUtils; switchFragment,Query,Mutationoutputs to*_NonReactimplementations when enabled.enabled.contents).RescriptRelayReact.*andRescriptRelayReact.loadQuery.@rescript/reactandreact-relayas optional peer deps for non‑React usage.CHANGELOG.md.Written by Cursor Bugbot for commit ff8c28a. This will update automatically on new commits. Configure here.