upgrade: timer package upgrade for Solid 2.0#843
Conversation
|
# Conflicts: # package.json # pnpm-lock.yaml
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@packages/timer/src/index.ts`:
- Around line 137-143: The effect in createTimeoutLoop exits early when
currTimeout === false so it stops watching timeout() and won’t restart when
timeout() later returns a number; instead, change the effect to always subscribe
to the timeout accessor (call timeout() at top to register) and conditionally
set/clear the interval: if currTimeout === false, return a cleanup that does
nothing but keeps the effect reactive; otherwise create the interval (using
setInterval and clearInterval) and ensure setCurrentTimeout(timeout()) is still
invoked inside handler; reference createTimeoutLoop, currTimeout, timeout(),
handler and setCurrentTimeout when making this change so the effect continues to
react to transitions from false -> number.
- Around line 88-113: The reconciliation path can cause the handler to be
scheduled twice because callHandler may set done=true inside the setTimeout
branch but subsequent code still starts a new timer; fix by guarding timer
creation with the done flag: (1) inside the reconcile setTimeout callback, only
assign mainId = timer(callHandler, currDelay) if done is false (ensure the
existing if (!done) check is present and correct around mainId = timer), and (2)
before creating the fallback id = timer(callHandler, currDelay) at the end of
the function, check if done is true and if so return a no-op cleanup instead of
starting a new timer. Reference symbols: callHandler, done, mainId, reconcileId,
id, timer, fractionDone, shouldHandleFraction.
🪄 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 Plus
Run ID: 4151e0a2-401a-4e60-9749-c5a461837749
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/timer/README.mdpackages/timer/package.jsonpackages/timer/src/index.tspackages/timer/test/index.test.ts
Reimplements the timer package for
solid-js@2.0.0.makeTimerno longer auto-registers withonCleanup— it returns a cleanup function and lets the caller decide.createTimerandcreateTimeoutLoopcallonCleanup(makeTimer(...))at their own call sites. This aligns with themake*/create*naming convention used across solid-primitives.isServeris now imported from@solidjs/web(moved out ofsolid-js/webin 2.0).createEffectupdated to the split compute/apply model:createEffect(compute, apply). The apply phase returns a cleanup function instead of usingonCleanup(which doesn't work in the apply phase).batchremoved — signal writes are auto-batched via microtask in 2.0. Tests useflush()where synchronous commitment is needed.createTimer(fractional delay carry-over) is rewritten to manage the full timer lifecycle directly in the apply cleanup closure, replacing the 1.x signal self-trigger trick.Bug fix:
createPolled— issue #808The original implementation used
createMemo(() => createSignal(fn(value), options))and calledmemo()[1](fn)from the timer. In production buildsmemo()[1]wasundefined, crashing on every timer tick.The fix captures the signal setter in a closure variable that is updated each time the memo re-runs:
I'm not fully confident in this
createPolledfix and would welcome a second look. The main concern is whether capturingsetin a closure and updating it on each memo re-run (dep change) is the right pattern in Solid 2.0, or whether there's a cleaner approach using the new writable derived signal (createSignal(computeFn, initialValue, options)) that avoids the TypeScript overload discrimination issues we ran into.Summary by CodeRabbit
Documentation
Chores