Conversation
sh-iv-am
requested review from
AbhishekChorotiya,
ArushKapoorJuspay,
aritro2002,
sakksham7 and
seekshiva
as code owners
🚫 Missing Linked IssueHi 👋 This pull request does not appear to be linked to any open issue yet. Linking your PR to an issue helps keep the project tidy and ensures the issue is closed automatically. ✔️ How to fix this
Once linked, this check will pass automatically on your next push or when you re-run the workflow. Thanks for helping maintainers! 🙌 |
| "@juspay-tech/hyper-js": "^1.7.2", | ||
| "@juspay-tech/react-hyper-js": "^1.2.4", | ||
| "@juspay-tech/hyper-js": "^2.1.0", | ||
| "@juspay-tech/react-hyper-js": "git@github.com:juspay/react-hyper-js.git", |
There was a problem hiding this comment.
added this as published version of react-hyper-js is not compatible with react 19
| let supportedCardBrands = React.useMemo(() => { | ||
| paymentMethodListValue->PaymentUtils.getSupportedCardBrands | ||
| }, paymentMethodListValue) | ||
| }, [paymentMethodListValue]) |
There was a problem hiding this comment.
I don't think so, it should never be outside of parentheses
| } | ||
|
|
||
| let rec transformKeysWithoutModifyingValue = (json: JSON.t, to: case) => { | ||
| let transformKeysWithoutModifyingValue = (json: JSON.t, to: case) => { |
There was a problem hiding this comment.
instead of removing rec, it should call transformKeysWithoutModifyingValue inside instead of transformKeys
There was a problem hiding this comment.
Not sure about functionality but getting Rescript compilation warning because of it.
Issues Found1. Bug — In // new (incorrect)
}, (
JotaiAtoms.areRequiredFieldsValid, // ← this is an atom, not a bool
isEmpty,
complete,
customerMethod,
JotaiAtoms.isManualRetryEnabled, // ← this is an atom, not a bool
))
// old — same issue existed via `open RecoilAtoms`
}, (areRequiredFieldsValid, isEmpty, complete, customerMethod, isManualRetryEnabled))These should be local variables bound via 2. Leftover commented-out code — open JotaiAtoms
// open JotaiAtomsV2 ← should be removedThe V2 atoms are accessed with full qualification ( 3. In external useSetAtom: atom<'value> => ('value => 'value) => unit = "useSetAtom"The return type forces callers to use 4. Demo app uses a git SSH URL dependency In "@juspay-tech/react-hyper-js": "git@github.com:juspay/react-hyper-js.git"This will fail for anyone without SSH access to that private repo. If this is temporary for testing, it should be reverted before merging. If it's a real change, it needs to be a published npm version. 5.
6. In type writableAtom<'value, 'args, 'result> = WritableAtom('value, 'args, 'result)This appears unused in the diff. Either remove it or document what it's intended for. VerdictThe migration is mechanically sound across all 100+ files, but there are a few things that should be addressed before merging — particularly the git SSH URL in the demo app (a potential CI/install blocker) and the |
🚫 Missing Linked IssueHi 👋 This pull request does not appear to be linked to any open issue yet. Linking your PR to an issue helps keep the project tidy and ensures the issue is closed automatically. ✔️ How to fix this
Once linked, this check will pass automatically on your next push or when you re-run the workflow. Thanks for helping maintainers! 🙌 |
2 participants
Type of Change
Description
Recoil to Jotai migration
How did you test it?
In example app with React 19
Checklist
npm run re:build