Skip to content

new: notification packages upgrade for Solid 2.0#893

Open
davedbase wants to merge 10 commits into
solidjs-community:nextfrom
davedbase:update/v2/notification
Open

new: notification packages upgrade for Solid 2.0#893
davedbase wants to merge 10 commits into
solidjs-community:nextfrom
davedbase:update/v2/notification

Conversation

@davedbase
Copy link
Copy Markdown
Member

@davedbase davedbase commented May 11, 2026

Key design decisions:

  • makeNotification has no onCleanup inside — consistent with the make* non-reactive contract; callers register close with onCleanup themselves
  • createNotification accepts a third handlers argument (NotificationEventHandlers) for onClick, onClose, and onError callbacks; all listeners are removed on close()
  • createNotificationPermission delegates to @solid-primitives/permission and surfaces the Permissions API's "prompt" vocabulary (vs Notifications API's "default")
  • Dev-mode console.warn when show() is called without permission

What this migrates

@solid-primitives/permission is updated to Solid 2.0 beta.10:

  • isServer import moved from solid-js/web@solidjs/web
  • on() removed; replaced with split createEffect(compute, apply)
  • Both signals use { ownedWrite: true } to allow writes from async Promise callbacks and event listeners outside a reactive owner
  • onCleanup registered at function scope (not inside the split effect apply, which has no reactive owner in beta.10) via a closure variable
  • Tests rewritten to read signals directly with flush() rather than capturing via createEffect

Merge note

Incorporates upstream/update/v2/permission (Alex Lohr's beta.8 migration). Conflict resolution kept our beta.10-compatible implementation throughout; the upstream's changeset (.changeset/sweet-olives-talk.md, marking permission as a major update) was taken as-is.

Files changed

  • New: 9 files under packages/notification/ (src, test × 2, dev, README, package.json, tsconfig, LICENSE, changeset)
  • Modified: packages/permission/ src, test, package.json, README
  • Modified: `pnpm-lock.yaml

Summary by CodeRabbit

  • New Features

    • Initial release of notification primitives package with browser notification support, permission state management, and reactive handlers.
  • Improvements

    • Updated permission primitives package to Solid 2.0.
  • Documentation

    • Added comprehensive documentation and examples for notification and permission packages.

Review Change Stack

atk and others added 4 commits April 28, 2026 15:44
- Add @solid-primitives/notification package (stage 0) with
  isNotificationSupported, makeNotification, createNotification,
  createNotificationPermission
- Migrate @solid-primitives/permission to Solid 2.0 beta.10:
  isServer from @solidjs/web, ownedWrite on signals, split createEffect
  with closure-based cleanup, onCleanup at function scope

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Takes the upstream changeset (.changeset/sweet-olives-talk.md) for the
permission package Solid 2.0 migration. Conflict resolution kept our
beta.10-compatible implementation: isServer from @solidjs/web, ownedWrite
on signals, and closure-based effect cleanup (vs upstream's beta.8 approach).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@davedbase davedbase added this to the Solid 2.0 Migration milestone May 11, 2026
@davedbase davedbase marked this pull request as ready for review May 17, 2026 13:21
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 17, 2026

🦋 Changeset detected

Latest commit: c136eba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@solid-primitives/notification Minor
@solid-primitives/permission Major

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

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 66b6c731-7978-44c2-970e-6bdc369eb624

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/notification-initial.md:
- Around line 12-14: Update the release note to match the shipped
implementation: change the server/unsupported fallback for
createNotificationPermission() from "denied" to "unknown" (the value returned by
the permission accessor when Notification is unsupported) and correct the peer
dependency versions from `solid-js@^2.0.0-beta.10` and
`@solidjs/web@^2.0.0-beta.10` to the actual `beta.14` pins used by the package
so the note accurately reflects createNotificationPermission() behavior and peer
deps.

In `@packages/notification/README.md`:
- Around line 97-104: The README examples are missing required Solid imports;
update the snippets to import Solid primitives used so they are copy-paste
runnable by adding imports for createEffect and any other primitives used (e.g.,
Show, Component) at the top of each example block; locate examples referencing
createEffect, Show, and Component and add corresponding import statements (e.g.,
import { createEffect, Show, Component } from "solid-js" or split into separate
imports) so each snippet is self-contained and runnable.
- Around line 116-131: The README incorrectly states that requestPermission
returns a NotificationPermission value and that permission changes from
"unknown" to "denied"; update the docs to match the actual API: note that
createNotificationPermission.permission() yields "unknown" on the server or when
the Notification API is unavailable and that requestPermission() resolves to
Promise<void> (it does not return "granted"|"denied"|"default"). Edit the
example and the descriptive text around createNotificationPermission,
permission, and requestPermission to remove the incorrect return-value
example/await usage and instead show the correct usage (gate UI on permission()
and call requestPermission() without expecting a returned
NotificationPermission).

In `@packages/notification/src/index.ts`:
- Around line 197-212: Update the JSDoc `@returns` to match the actual return
shape of createNotificationPermission: include `pending` alongside `permission`
and `requestPermission` so the doc reads something like `{ permission,
requestPermission, pending }`; locate the JSDoc block above the
createNotificationPermission function and add `pending` to the documented return
contract referencing the same symbol name `pending`.
🪄 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: 92494dd7-d06c-4116-a371-201281f45815

📥 Commits

Reviewing files that changed from the base of the PR and between 76d6f2e and e7f52d3.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (14)
  • .changeset/notification-initial.md
  • .changeset/sweet-olives-talk.md
  • packages/notification/LICENSE
  • packages/notification/README.md
  • packages/notification/dev/index.tsx
  • packages/notification/package.json
  • packages/notification/src/index.ts
  • packages/notification/test/index.test.ts
  • packages/notification/test/server.test.ts
  • packages/notification/tsconfig.json
  • packages/permission/README.md
  • packages/permission/package.json
  • packages/permission/src/index.ts
  • packages/permission/test/index.test.ts

Comment thread .changeset/notification-initial.md Outdated
Comment thread packages/notification/README.md
Comment thread packages/notification/README.md Outdated
Comment thread packages/notification/src/index.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants