new: analytics package for Solid 2.0#925
Open
davedbase wants to merge 8 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 53e2cf8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
packages/analytics/src/guard.ts (1)
19-30: ⚡ Quick winNavigation can stall indefinitely if
drain()never settles.
onBeforeLeaveprevents navigation and only retries insidedrain().finally(...). If any in-flight plugin promise hangs (e.g. a network request without its own timeout),drain()never settles and the user is permanently blocked from navigating. Consider racingdrain()against a bounded timeout so navigation always proceeds:♻️ Suggested approach
- void analytics.drain().finally(() => { + const timeout = new Promise<void>(r => setTimeout(r, 2000)); + void Promise.race([analytics.drain(), timeout]).finally(() => { guarding = false; event.retry(true); });🤖 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 `@packages/analytics/src/guard.ts` around lines 19 - 30, The onBeforeLeave handler in buildOnBeforeLeave currently blocks navigation until analytics.drain() settles, which can hang indefinitely; modify onBeforeLeave to race analytics.drain() with a bounded timeout (e.g., Promise.race([analytics.drain(), timeoutPromise])) and ensure the finally-like cleanup always runs: clear guarding and call event.retry(true) after the race regardless of which wins, and cancel/prevent duplicate retries by keeping the guarding flag logic around onBeforeLeave and the returned promise resolution path.
🤖 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/analytics/package.json`:
- Around line 54-63: primitive.list in packages/analytics/package.json is out of
sync: remove the non-existent "createEventHandler" entry and add the actual
relay plugin primitives "createSolidStartRelayPlugin" and
"createTanStackRelayPlugin" (these correspond to the ./relay/solidstart and
./relay/tanstack exports); update the "list" array so it matches the real
exported API names (e.g., keep "makeAnalytics", "createAnalytics",
"AnalyticsProvider", "useAnalytics", "makeAnalyticsGuard",
"createAnalyticsGuard", "createServerPlugin" and replace "createEventHandler"
with "createSolidStartRelayPlugin" and "createTanStackRelayPlugin") so subpath
exports and primitive.list stay consistent.
In `@packages/analytics/README.md`:
- Around line 189-191: The fenced code block containing "browser →
createServerPlugin → your server function → GA / Mixpanel / Segment" is missing
a language identifier and triggers markdownlint MD040; update that markdown
fence to include a language (e.g., change ``` to ```text) so the block is
explicitly marked as text.
In `@packages/analytics/src/analytics.ts`:
- Around line 105-113: drainBatch currently applies drainSize unconditionally
which causes leftover events when batching is disabled; change logic so
drainSize is only used when batching is true — i.e., in drainBatch (or its
caller drainQueue) check the batching flag and set max = batching ? (drainSize
?? Infinity) : Infinity (or skip using drainSize entirely when batching is
false) so that initPlugin/startPoll draining (called when allReady) will remove
the entire queue (using queue.remove) and not leave events stranded before
dispatch bypasses the queue.
In `@packages/analytics/src/context.ts`:
- Around line 34-45: The useAnalytics hook currently returns
useContext(AnalyticsCtx) directly which can be undefined when no provider
exists, making the ReactiveAnalyticsControls return type unsound and
contradicting the JSDoc; change useAnalytics to call useContext(AnalyticsCtx),
store the result in a local (e.g., controls), if controls is undefined throw
ContextNotFoundError (or construct a helpful error message) and otherwise return
controls so the runtime matches the declared ReactiveAnalyticsControls type and
the documented behavior; reference the useAnalytics function, AnalyticsCtx,
ReactiveAnalyticsControls, and ContextNotFoundError when making this change.
In `@packages/analytics/src/guard.ts`:
- Around line 38-43: The JSDoc example redeclares cleanup in the same scope;
change one of the identifiers to avoid collision (e.g., rename the cleanup
returned from makeAnalyticsGuard to cleanupGuard or guardCleanup) so the example
compiles; update the example lines that destructure makeAnalytics and
makeAnalyticsGuard (referencing makeAnalytics and makeAnalyticsGuard) and any
subsequent usage to use the new name.
In `@packages/analytics/src/relay/solidstart.ts`:
- Around line 11-29: The documented SolidStart `action` example (relayEvent)
returns Promise<void>, which triggers Solid Router page query revalidation;
update the example in the docs to explicitly opt out of revalidation by
returning a response with revalidate: [] (e.g., return json(..., { revalidate:
[] }) or return reload({ revalidate: [] })) from the action after awaiting
ga.track, and also relax the typings on createSolidStartRelayPlugin /
createServerPlugin to accept broader return types (e.g., Promise<unknown> or
Promise<any>) instead of Promise<void> so the example type-checks.
In `@packages/analytics/src/relay/tanstack.ts`:
- Around line 11-30: Update the JSDoc example so the Solid-specific import and
validation API are correct: change the import of createServerFn to come from
"`@tanstack/solid-start`" instead of "`@tanstack/start`", and replace the chained
.validator(...) call with .inputValidator(...) on the createServerFn returned
object (referenced as createServerFn and relayEvent in the example); keep the
rest of the snippet (AnyPayload, googleAnalytics, createTanStackRelayPlugin)
unchanged so the example is copy-paste safe for Solid users.
In `@packages/analytics/test/index.test.ts`:
- Around line 12-14: Tests in packages/analytics/test/index.test.ts rely on real
timers via the delay() helper which makes CI flaky; switch to Vitest fake timers
by calling vi.useFakeTimers() at the start of each test (and vi.useRealTimers()
in teardown, or restore timers in existing afterEach(() => dispose?.()) blocks)
and replace all uses of await delay(n) or delay(n).then(...) with await
vi.advanceTimersByTimeAsync(n) (use await vi.advanceTimersByTimeAsync(0) where
delay() was called with no arg); update the listed waits
(23,36,51,74,78,89,101,116,129,146,160,163,174,211,276,311,314,329,333,349,365,396,422)
accordingly and ensure any setInterval/poll/drain logic in analytics.ts runs by
advancing timers instead of real waiting.
In `@packages/utils/src/index.ts`:
- Around line 105-109: The docs/typing overstate global uniqueness: update the
EventMeta.rid description and any README text to say the ID from
createIdGenerator()/generateId is unique per analytics instance/generator (not
globally across separate clients/SSR requests); alternatively, to harden
cross-client uniqueness modify createIdGenerator to append a small random
segment (e.g., hex or base36 random 4-8 chars via crypto or Math.random) to the
`${Date.now().toString(36)}-${(++seq).toString(36)}` output so rid becomes
`${ts}-${seq}-${rand}`, and ensure any references to generateId in
packages/analytics/src/analytics.ts and the EventMeta type/comment are updated
to reflect the change.
---
Nitpick comments:
In `@packages/analytics/src/guard.ts`:
- Around line 19-30: The onBeforeLeave handler in buildOnBeforeLeave currently
blocks navigation until analytics.drain() settles, which can hang indefinitely;
modify onBeforeLeave to race analytics.drain() with a bounded timeout (e.g.,
Promise.race([analytics.drain(), timeoutPromise])) and ensure the finally-like
cleanup always runs: clear guarding and call event.retry(true) after the race
regardless of which wins, and cancel/prevent duplicate retries by keeping the
guarding flag logic around onBeforeLeave and the returned promise resolution
path.
🪄 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: 638db1dc-28a2-4642-9265-1187c59e3946
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.changeset/analytics-solid2-redesign.mdpackages/analytics/README.mdpackages/analytics/package.jsonpackages/analytics/src/analytics.tspackages/analytics/src/context.tspackages/analytics/src/ga.tspackages/analytics/src/guard.tspackages/analytics/src/index.tspackages/analytics/src/relay.tspackages/analytics/src/relay/solidstart.tspackages/analytics/src/relay/tanstack.tspackages/analytics/src/types.tspackages/analytics/stories/analytics.stories.tsxpackages/analytics/test/index.test.tspackages/analytics/test/server.test.tspackages/analytics/tsconfig.jsonpackages/queue/src/task-queue.tspackages/utils/src/index.ts
💤 Files with no reviewable changes (1)
- packages/analytics/src/ga.ts
# Conflicts: # pnpm-lock.yaml
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.
Adds a new
analyticspackage providing a queue-based analytics dispatch pipeline for Solid 2.0, with full SSR support, reactive signals, context sharing, navigation guards, and server relay. This should be considered an experimental package because it's trying something pretty unique. It needs a lot of work.I'm thinking of removing or rethinking the leave guard.
What's included
Core primitives
makeAnalytics(plugins, options?)— non-reactive base primitive; returns[controls, cleanup]. Works at module level, in server functions, anywhere.createAnalytics(plugins, options?)— reactive wrapper; auto-disposes with the owner, exposespendingCount: Accessor<number>tracking queued events while plugins initialize.AnalyticsProvider/useAnalytics()— context pair for sharing a single analytics instance across the component tree.Navigation guards
makeAnalyticsGuard/createAnalyticsGuard— pauses SolidJS Router navigation until all in-flight dispatches settle, preventing events from being lost on route change. Also registers abeforeunloadsafety net.Server relay
createServerPlugin(/relay) — client-side plugin that forwards events to a server function, keeping provider API keys off the client.createSolidStartRelayPlugin(/relay/solidstart) — thin wrapper for wiring to a SolidStartaction.createTanStackRelayPlugin(/relay/tanstack) — wraps a TanStack StartcreateServerFn, handling the{ data }call signature automatically.Design decisions
analyticsnpm package — any existing plugin works without modification.abort()to stop the chain for that event.createAnalyticsdelegates tomakeAnalyticsvia an optionalonQueueChangecallback — no logic duplication.@solid-primitives/queue'smakeQueueinternally for the event buffer.page()omitswindow-dependent defaults on the server; guards are no-ops in SSR.Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores