upgrade: devices and sensors package upgrade for Solid 2.0#905
upgrade: devices and sensors package upgrade for Solid 2.0#905davedbase wants to merge 11 commits into
Conversation
🦋 Changeset detectedLatest commit: 0e03082 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/sensors/src/sensor.ts (1)
43-49: ⚡ Quick winDowngrade:
sensor.start()permission failures are handled via the sensorerrorevent, not a synchronous throw; current “null only on constructor failure” contract matches docs.
- The JSDoc/README promise
nullonly when the constructor throws; they don’t claimstart()exceptions are part of that fallback.- Generic Sensor API runtime permission denials during
start()are typically reported asynchronously via the sensor’serrorevent (e.g.,NotAllowedError), so missing atry/catcharoundstart()shouldn’t break the stated fallback behavior.- Optional hardening: if
start()can ever throw synchronously for other reasons, wrapping it and reusing the existing listener cleanup would improve resilience.🤖 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/sensors/src/sensor.ts` around lines 43 - 49, The current code relies on the sensor "error" event for permission failures but does not guard against the (rare) case that sensor.start() may throw synchronously; wrap the call to sensor.start() in a try/catch so that if start() throws you still run the same cleanup logic (removeEventListener(handleReading) and sensor.stop()) and optionally rethrow or call onChange(null) as your fallback contract requires; keep the existing listener setup/teardown (handleReading, sensor.addEventListener, sensor.removeEventListener, sensor.stop) and ensure the catch block reuses the same cleanup code to avoid leaking listeners or running a started sensor after an error.
🤖 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 @.changeset/sensors-new-package.md:
- Around line 5-10: The changeset summary only lists accelerometer/gyroscope
functions; update the summary to enumerate the full exported API by adding the
missing symbols: makeSensor, createSensor, makeCompass, createCompass,
makeBattery, createBattery (in addition to makeAccelerometer/createAccelerometer
and makeGyroscope/createGyroscope), and optionally note their brief behavior
(e.g., make* raw listener returning cleanup, create* reactive/store accessors)
so release notes fully reflect the package exports.
In `@packages/devices/package.json`:
- Around line 55-61: The package declares `@solidjs/web` only as a devDependency
but imports isServer from it in packages/devices/src/index.ts and the changeset
states it must be a peer dependency; update packages/devices/package.json so
that `@solidjs/web` is added to "peerDependencies" (use the same version spec as
the existing devDependency, e.g. "2.0.0-beta.13") and keep or remove the
devDependency as desired, ensuring the peerDependencies section for the
`@solid-primitives/devices` package includes "`@solidjs/web`" alongside "solid-js".
In `@packages/devices/README.md`:
- Around line 17-21: The installation code fence in packages/devices/README.md
is missing a language tag causing MD040 lint failures; update the opening fence
for the block containing "npm install `@solid-primitives/devices`" and "pnpm add
`@solid-primitives/devices`" to include a language tag (e.g., change ``` to
```bash) so the fenced code block is properly recognized by the markdown linter.
In `@packages/sensors/package.json`:
- Line 4: Update the package.json "description" value to accurately reflect the
current feature set: replace the existing text that only mentions "accelerometer
and gyroscope" with a concise summary that includes generic sensor, compass, and
battery primitives (e.g., mention "accelerometer, gyroscope, generic Sensor API,
compass, and battery" or similar) so the package metadata aligns with the code
in packages/sensors and improves discoverability.
In `@packages/sensors/README.md`:
- Around line 15-19: Update the installation code fence in the README so the
triple-backtick block that contains "npm install `@solid-primitives/sensors`" /
"pnpm add `@solid-primitives/sensors`" is a typed fence (e.g. change ``` to
```bash) to satisfy markdownlint and ensure proper syntax highlighting for that
installation snippet.
In `@packages/sensors/test/index.test.ts`:
- Around line 390-393: The test currently only checks that start() was called
but doesn't verify the constructor received the expected options; update the
constructor in the test helper (where constructor(_opts?: any) { super();
lastCompassInstance = this; }) to capture and store the incoming args (e.g. set
lastCompassInstance.constructorArgs = _opts or set a separate lastCompassArgs
variable) and then update the test titled “constructor options are passed” to
assert those stored constructor arguments equal the expected options in addition
to keeping the existing assertion that start() was called; reference the test
helper's constructor, lastCompassInstance, and the test that asserts start().
- Around line 650-673: The Promise executor is marked async which can mask
errors; remove async from new Promise<void>(async (resolve, reject) => { ... })
and instead explicitly schedule microtasks (e.g., use
Promise.resolve().then(...) or queueMicrotask) where the code currently does
await Promise.resolve(); keep the same logic around createBattery(),
createEffect(...), mockBattery.dispatchEvent(...), and the setTimeout timeout
reject — ensure dispose() and resolve() are called from the non-async executor
so thrown errors propagate to the Promise rejection instead of being swallowed
by an async executor.
---
Nitpick comments:
In `@packages/sensors/src/sensor.ts`:
- Around line 43-49: The current code relies on the sensor "error" event for
permission failures but does not guard against the (rare) case that
sensor.start() may throw synchronously; wrap the call to sensor.start() in a
try/catch so that if start() throws you still run the same cleanup logic
(removeEventListener(handleReading) and sensor.stop()) and optionally rethrow or
call onChange(null) as your fallback contract requires; keep the existing
listener setup/teardown (handleReading, sensor.addEventListener,
sensor.removeEventListener, sensor.stop) and ensure the catch block reuses the
same cleanup code to avoid leaking listeners or running a started sensor after
an error.
🪄 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: 52eed979-4069-4ae7-916e-b58484ad988f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/devices-solid2-migration.md.changeset/sensors-new-package.mdpackages/devices/README.mdpackages/devices/package.jsonpackages/devices/src/index.tspackages/devices/test/index.test.tspackages/devices/test/server.test.tspackages/sensors/CHANGELOG.mdpackages/sensors/README.mdpackages/sensors/package.jsonpackages/sensors/src/accelerometer.tspackages/sensors/src/battery.tspackages/sensors/src/compass.tspackages/sensors/src/gyroscope.tspackages/sensors/src/index.tspackages/sensors/src/sensor.tspackages/sensors/test/index.test.tspackages/sensors/test/server.test.tspackages/sensors/tsconfig.json
…d-primitives into update/v2/devices # Conflicts: # pnpm-lock.yaml
PR separates devices into devices and sensors. Adds new sensor capabilities by expanding on gyroscope and basic accelerometer primitives that already existed.
Updated:
@solid-primitives/devicesv2.0.0Breaking changes:
createAccelerometerandcreateGyroscopeare removed — use@solid-primitives/sensorsinstead^2.0.0-beta.13Internals:
createEffectcalls converted to Solid 2.0 split(compute, apply)formcreateMemocalls updated (second arg wasinitialValuein 1.x, nowoptions)isServerimport moved fromsolid-js/webto@solidjs/webcreateStoreimport moved fromsolid-js/storetosolid-js--
New package:
@solid-primitives/sensorsv0.1.0Each sensor follows the Solid Primitives
make*/create*convention:make*— attaches listeners and returns a cleanup function; no Solid lifecycle dependencycreate*— wraps in signals, registers cleanup viaonCleanup, SSR-safeAccelerometer (
DeviceMotionEvent)makeAccelerometer(onChange, { includeGravity?, interval? })— throttled, leading-edgecreateAccelerometer(includeGravity?, interval?)→Accessor<AccelerometerReading | undefined>Gyroscope (
DeviceOrientationEvent)makeGyroscope(onChange, { interval? })— null orientation values coerced to 0createGyroscope(interval?)→ reactive{ alpha, beta, gamma }getter objectGeneric Sensor API (Chromium; covers
LinearAccelerationSensor,GravitySensor,AbsoluteOrientationSensor, etc.)makeSensor(SensorClass, onChange, options?)→VoidFunction | nullcreateSensor(SensorClass, options?)→Accessor<T | undefined>— uses{ equals: false }so every reading fires the effect even when the sensor object reference is unchangedCompass (
window.Magnetometervia Generic Sensor API)makeCompass(onChange, { frequency?, referenceFrame? })→VoidFunction | nullcreateCompass(options?)→ reactive{ x, y, z }getter object (µT, null coerced to 0)Battery (
navigator.getBattery())makeBattery(onChange)— async init, synchronous cleanup; safe to pass directly toonCleanup; pre-dispose race handledcreateBattery()→Accessor<BatteryReading | undefined>— startsundefineduntil the API resolvesSource is split into one file per sensor (
accelerometer.ts,gyroscope.ts,sensor.ts,compass.ts,battery.ts) withindex.tsre-exporting all of themSummary by CodeRabbit
New Features
@solid-primitives/sensorspackage providing device motion, orientation, compass, and battery sensor support through reactive APIs.Breaking Changes
@solid-primitives/devicesv2.0: now focuses on media device enumeration; accelerometer and gyroscope features moved to sensors package.Documentation