refactor: migrate source from Flow to TypeScript#805
Merged
Conversation
Convert lib/*.js (Flow) to TypeScript (.ts/.tsx) and replace the Babel+Flow toolchain with tsup (CJS+ESM+auto-generated .d.ts), keeping webpack for the UMD bundle. Types are now generated from source instead of hand-maintained in typings/index.d.ts, eliminating Flow/TS drift. Public API is unchanged (minor-version-safe): CJS keeps module.exports===Draggable plus .default and .DraggableCore (PR #254/#266); UMD bundle still exposes global ReactDraggable with react/react-dom externals; generated .d.ts matches the previously-shipped surface and the unchanged typings/test.tsx compiles against it. DraggableCore children stays React.ReactNode in the public types (the internal Flow type was ReactElement) with a regression guard in test/typeCompat. Adds tsup.config.ts, root tsconfig.json, eslint @typescript-eslint, native-TS vitest, and extra unit tests (198 pass, up from 131). No version bump or CHANGELOG change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR migrates react-draggable from Flow/Babel to TypeScript, adds tsup-based builds and CJS/UMD compatibility shims, refactors components and utilities to TypeScript, updates lint/test/build tooling, and introduces type-compat and expanded unit tests. ChangesFlow to TypeScript Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
scripts/verify-build.cjsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 14-17: The lint target currently runs "tsc -p typings" but that
tsconfig depends on a generated declaration (build/cjs/cjs.d.ts), causing
non-deterministic failures; update the Makefile's lint target (the lint rule
that runs "$(BIN)/eslint lib", "$(BIN)/tsc --noEmit", "$(BIN)/tsc -p typings")
to first produce the required declarations (either by invoking the build step
that creates build/cjs/cjs.d.ts or by running tsc with emitDeclarationOnly
against the appropriate tsconfig) so that the subsequent "$(BIN)/tsc -p typings"
always has the generated artifact available. Ensure the change references the
same lint target and the "$(BIN)/tsc -p typings" invocation so reviewers can
locate and verify the fix.
In `@test/typeCompat.test.ts`:
- Around line 50-56: The beforeAll hook in typeCompat.test.ts currently
hard-fails when the built declaration (generatedDts) is missing; change it so
the suite is build-aware: inside beforeAll (the function referencing existsSync
and generatedDts) attempt to produce the artefact (e.g., run a synchronous build
command like yarn build via child_process.execSync or the repo's build script)
if the file is absent, then re-check existsSync; if the file still isn't
present, skip the suite or fail with a clear message (use the test framework's
skip mechanism or a controlled throw) rather than unconditionally throwing at
first sight.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbd92e1c-a5aa-4477-afbb-62af0baf8930
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.babelrc.js.flowconfigMakefileeslint.config.mjslib/Draggable.tsxlib/DraggableCore.tsxlib/cjs.jslib/cjs.tslib/umd.tslib/utils/domFns.tslib/utils/getPrefix.tslib/utils/log.tslib/utils/positionFns.tslib/utils/shims.tslib/utils/types.tspackage.jsontest/typeCompat.test.tstest/typeCompat/fixture.tsxtest/typeCompat/tsconfig.jsontest/utils/domFns.extra.test.jstest/utils/getPrefix.extra.test.jstest/utils/positionFns.bounds.test.jstest/utils/shims.extra.test.jstsconfig.jsontsup.config.tstypings/index.d.tstypings/tsconfig.jsonvitest.config.jswebpack.config.js
💤 Files with no reviewable changes (4)
- lib/cjs.js
- typings/index.d.ts
- .flowconfig
- .babelrc.js
Comment on lines
14
to
17
| lint: | ||
| @$(BIN)/flow | ||
| @$(BIN)/eslint lib/* lib/utils/* | ||
| @$(BIN)/eslint lib | ||
| @$(BIN)/tsc --noEmit | ||
| @$(BIN)/tsc -p typings |
There was a problem hiding this comment.
Make lint deterministic against generated declarations.
Line 17 now depends on generated build/cjs/cjs.d.ts (via typings/tsconfig.json), but lint does not ensure that artifact exists. This can fail on clean environments and pass only on machines with stale build output.
🔧 Proposed fix
-lint:
+lint: build-lib
@$(BIN)/eslint lib
@$(BIN)/tsc --noEmit
@$(BIN)/tsc -p typings📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lint: | |
| @$(BIN)/flow | |
| @$(BIN)/eslint lib/* lib/utils/* | |
| @$(BIN)/eslint lib | |
| @$(BIN)/tsc --noEmit | |
| @$(BIN)/tsc -p typings | |
| lint: build-lib | |
| @$(BIN)/eslint lib | |
| @$(BIN)/tsc --noEmit | |
| @$(BIN)/tsc -p typings |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 14 - 17, The lint target currently runs "tsc -p
typings" but that tsconfig depends on a generated declaration
(build/cjs/cjs.d.ts), causing non-deterministic failures; update the Makefile's
lint target (the lint rule that runs "$(BIN)/eslint lib", "$(BIN)/tsc --noEmit",
"$(BIN)/tsc -p typings") to first produce the required declarations (either by
invoking the build step that creates build/cjs/cjs.d.ts or by running tsc with
emitDeclarationOnly against the appropriate tsconfig) so that the subsequent
"$(BIN)/tsc -p typings" always has the generated artifact available. Ensure the
change references the same lint target and the "$(BIN)/tsc -p typings"
invocation so reviewers can locate and verify the fix.
CI was red because the migration left three build-order assumptions that only surfaced in a fresh checkout: the typecheck job still ran the removed `yarn flow`; `make lint` and the typeCompat test resolved `react-draggable` to the generated build/cjs/cjs.d.ts, which doesn't exist before `yarn build`. The workflow's own verify ran against a pre-populated build/, hiding it. Repoint typings/tsconfig.json and test/typeCompat/tsconfig.json at the source entry (lib/cjs.ts) instead of the built declaration — same public surface, no build dependency. Drop the build-exists guard in typeCompat.test.ts. Replace the dead `yarn flow` CI step with `yarn tsc --noEmit`. Verified lint/typecheck/test/coverage/build all green with build/ absent.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/typeCompat.test.ts (1)
56-60:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncrease test timeout to accommodate TypeScript compilation.
This test is timing out in CI after 5000ms. TypeScript compilation of the
typingsproject takes longer than the default Vitest timeout, especially on CI runners.Add a timeout parameter to allow sufficient time for
tscto complete.⏱️ Proposed fix
- it('the existing typings/test.tsx still compiles unchanged against the public surface', () => { + it('the existing typings/test.tsx still compiles unchanged against the public surface', () => { const {ok, output} = runTsc(resolve(repoRoot, 'typings')); expect(output, output).toBe(''); expect(ok).toBe(true); - }); + }, 30000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/typeCompat.test.ts` around lines 56 - 60, The test times out because tsc can take longer than Vitest's default; update the test in typeCompat.test.ts that calls runTsc(resolve(repoRoot, 'typings')) to pass a longer timeout to the it() call (e.g., add a third or second argument timeout value to it(...) or use the optional timeout parameter signature) so the test has enough time to complete in CI while keeping the existing assertions using runTsc, resolve, repoRoot, and output unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/typeCompat.test.ts`:
- Around line 50-54: The TypeScript compile test can exceed Vitest's default
timeout; update the two test cases (including the one named "public surface is
API-compatible with the old hand-written surface") to pass an increased timeout
value (e.g., 20000 ms) as the third argument to the it(...) call so tsc has
enough time in CI; locate the tests around runTsc(...) and add the timeout
parameter to both it invocations.
---
Outside diff comments:
In `@test/typeCompat.test.ts`:
- Around line 56-60: The test times out because tsc can take longer than
Vitest's default; update the test in typeCompat.test.ts that calls
runTsc(resolve(repoRoot, 'typings')) to pass a longer timeout to the it() call
(e.g., add a third or second argument timeout value to it(...) or use the
optional timeout parameter signature) so the test has enough time to complete in
CI while keeping the existing assertions using runTsc, resolve, repoRoot, and
output unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d5dcfa2-e775-4e7a-9b90-cea3f5bce7ac
📒 Files selected for processing (4)
.github/workflows/ci.ymltest/typeCompat.test.tstest/typeCompat/tsconfig.jsontypings/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/typeCompat/tsconfig.json
The type-compat cases shell out to a full tsc compile of the source type graph (~6s on Node 20/22 CI runners), exceeding vitest's 5s default and timing out; the faster Node 24 runner masked it. Give both cases a 60s timeout, matching the project's other slow (browser) suites.
|
Actionable comments posted: 0 |
Quality cleanups from a /simplify pass, no behavior or API change: move the public DraggableEvent union into lib/utils/types.ts alongside the other public types (it was the lone public type defined in the entry file) and re-export it from cjs.ts; replace 'as Pick<DraggableState, keyof DraggableState>' with the equivalent 'as DraggableState'; drop DraggableCore's redundant propTypes index-signature annotation to match Draggable; remove inert declaration/emitDeclarationOnly/outDir keys from the root tsconfig (noEmit wins; tsup owns dts emit); fix a stale fixture comment that described build-artifact validation after it was repointed at source.
|
Actionable comments posted: 0 |
lib/cjs.ts instructs verifying the module.exports===Draggable shape with a runtime require() check, but nothing enforced it — the historical CJS dual-export contract (PR #254/#266) and the UMD global could regress invisibly and only surface when a consumer installs the package. Add scripts/verify-build.cjs: requires the built CJS and asserts root===Draggable, root===.default, and .DraggableCore present; asserts the UMD bundle exposes global ReactDraggable. Wired into the Makefile build target's recipe so it runs after build-lib + build-web on both `yarn build` and `make build`. Verified it fails on a corrupted export shape. Not build-order-coupled to the unit-test matrix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the library source from Flow to TypeScript with zero public-API changes, so it can ship as a minor release.
lib/*.js(Flow) becomeslib/*.ts/*.tsx, the Babel+Flow toolchain is replaced by tsup (CJS + ESM + auto-generated.d.ts), and webpack is retained only for the UMD bundle. TypeScript types are now generated from source instead of hand-maintained intypings/index.d.ts, eliminating the long-standing Flow↔TS drift problem.No
package.jsonversion bump and noCHANGELOG.mdentry are included — this PR is the migration only; releasing is a separate step.Why now
The codebase carried two parallel type systems (Flow internally, hand-written TS definitions shipped to consumers) that had to be kept in sync by hand. Consolidating on TypeScript with generated declarations removes that maintenance burden and the drift risk.
What changed
lib/{Draggable,DraggableCore}.{js→tsx},lib/utils/*.{js→ts},lib/cjs.{js→ts}, newlib/umd.ts.// @flowremoved; Flow syntax translated to TS. Runtime logic untouched.tsup.config.tsemits CJS+ESM+.d.tsintobuild/cjs; webpack still emits the UMD globalbuild/web/react-draggable.min.js..babelrc.jsand.flowconfigremoved;Makefileupdated.typings/index.d.tsdeleted; declarations now generated.typings/tsconfig.jsonrepointed at the generated declaration andtypings/test.tsxleft unchanged as a compile-time contract check.@typescript-eslint; Vitest now handles TS natively (Babel-Flow transform removed). Addedtest/typeCompat/(asserts the generated public surface matches the pre-migration one) plus extra unit tests.Compatibility (minor-version safe)
Verified against
masterHEAD — the migration introduces no behavior or public-surface change:require()returnsmodule.exports === Draggable, with.default === Draggableand.DraggableCore(preserves PR make Draggable explicit default export #254 / Draggable is undefined with normal import syntax in TypeScript #266). Confirmed at runtime against the built artifact.ReactDraggablewithreact/react-dommapped toReact/ReactDOMexternals..d.tsexports the same surface (Draggabledefault,DraggableCore,DraggableProps,DraggableCoreProps,DraggableBounds,DraggableData,DraggableEvent,DraggableEventHandler,ControlPosition,PositionOffsetControlPosition); the unchangedtypings/test.tsxcompiles against it.DraggableCorechildrenstaysReact.ReactNode(the internal Flow type wasReactElement) — guarded by a regression case intest/typeCompat.Status — 2026-05-28
tsc --noEmit(typecheck)yarn lint(eslint + tsc + typings tsc)yarn test(vitest)yarn build(tsup + webpack)build/cjs/*+build/web/react-draggable.min.jsmodule.exports===Draggable,.default,.DraggableCoreReactDraggable, react/react-dom external.d.tsvs old surfacetypings/test.tsxcompilesAll CI jobs green on the latest commit: lint, typecheck, test (Node 20/22/24), test-browser (Puppeteer), coverage, plus CodeRabbit.
CI fixes applied after open (post-migration follow-ups)
The initial push was red because the migration left build-order assumptions the workflow's own verify (run against a pre-populated
build/) had masked:b42284d— typecheck job still ran the removedyarn flow;make lintand the type-compat test resolvedreact-draggableto the generatedbuild/cjs/cjs.d.ts, absent in a fresh checkout. Repointed type validation at the source entry (lib/cjs.ts) — same public surface, no build dependency — and replaced the deadflowstep withtsc --noEmit.5c8762e— the type-compat test shells out to a fulltsccompile (~6s on Node 20/22 runners), exceeding vitest's 5s default and timing out (Node 24 masked it). Raised the per-case timeout to 60s.Open follow-ups (resume here)
ctrl+click(fix: treat ctrl+click as right-click on macOS #786) and React 19findDOMNodechanges that land in the same release.Test plan
yarn lintyarn test(198 pass)yarn build+ runtime CJS/UMD/.d.tsshape verificationyarn test:browser(Puppeteer, green in CI)Summary by CodeRabbit