Migrate Cropper to Hooks and React Compiler - #664
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
ValentinH
left a comment
There was a problem hiding this comment.
Standards
No documented standards; no hard violations. All findings are baseline judgement calls:
-
Repeated Switches —
src/useCropper.tsx:1184-1228: both handlers repeatswitch (event.key)withcase 'ArrowUp','ArrowDown','ArrowLeft', and'ArrowRight'. Define one arrow-delta map/type guard and reuse it. -
Duplicated Code —
src/useCropper.tsx:1116-1144:function setImageElement(...)andfunction setVideoElement(...)repeatruntime.media = element,const refCleanup = assignRef(mediaRef, element), and equivalent cleanup. Extract a typed media-ref callback factory/helper. -
Data Clumps —
src/useCropper.tsx:102-103,358-359,1052-1061:cropSizeWidth?: numberandcropSizeHeight?: numberalways travel and compare together, includingprevious.cropSizeWidth !== requestedCropSize?.widthand the matching height check. StorecropSize?: SizeinCommittedInputsand compare throughsizesEqual; scalar effect dependencies may remain separate. -
Duplicated Code —
tsdown.config.mts:29-32,vite.config.mts:8-11,vitest.config.mts:7-11: all repeatpresets: [reactCompilerPreset({ target: '19', panicThreshold: 'all_errors' })]. Centralize the compiler options/preset factory so build, demo, and tests cannot drift.
Spec
-
Incorrect implementation — “Use all the best practices” / “Rely on the React compiler…”:
src/useCropper.tsx:616-903creates Effect Events subsequently passed torequestAnimationFrame, timers, and document listeners from ordinary interaction paths entered via JSX handlers atsrc/useCropper.tsx:1155-1171. React permits Effect Events only from Effects or other Effect Events; theReact.useEffectEventnamespace spelling currently bypasses the hooks linter's call-site check. Route these async callbacks through effect-owned subscriptions (or a non-Effect-Event latest-handler primitive) and reserve Effect Events for effect-owned calls. -
Partial migration — “Today is the day we finally migrate to Hooks.”:
docs/package.json:22still installsreact-easy-crop5.5.6. The deployed documentation examples import that package, so they continue running the old class implementation while the docs describe v7 behavior; the new docs CI build validates neither the hooks engine nor its stylesheet/callback changes. Make the docs consume the local v7 build/workspace package.
Summary: Standards — 4 judgement-call findings; worst: duplicated Compiler configuration can drift across build, demo, and tests. Spec — 2 findings; worst: interaction-path Effect Events violate React's call-site contract.
Summary
Breaking changes
Verification
Release: major.
📦 Published PR as canary version:
7.0.0--canary.664.2c66a6d.0✨ Test out this PR locally via:
npm install react-easy-crop@7.0.0--canary.664.2c66a6d.0 # or yarn add react-easy-crop@7.0.0--canary.664.2c66a6d.0