Skip to content

Delay subscribing to a component until its fully mounted - #794

Open
JoviDeCroock wants to merge 6 commits into
mainfrom
fix-react-suspense
Open

Delay subscribing to a component until its fully mounted#794
JoviDeCroock wants to merge 6 commits into
mainfrom
fix-react-suspense

Conversation

@JoviDeCroock

@JoviDeCroock JoviDeCroock commented Nov 12, 2025

Copy link
Copy Markdown
Member

Resolves #768

This effectively addresses the "missing lifecycle" problem of React, when React suspends during mount it will discard any in progress work which means that the store won't be disposed and hence the subscriptions/... won't be discarded. We circumvent this by overriding the Signal _subscribe method, which sets up the subscriptions, to track all subscriptions for a store and only subscribe them when we are sure the component is fully mounted. This prevents us from subscribing on a work-in-progress state which will just be discarded.

Why do I think this is breaking? React already has a rule not to setState during render but we can't be sure everyone expects that, hence we are marking this as breaking. No synchronous work should be able to intervene during component rendering hence this forking of Signal.prototype._subscribe is safe as long as we ensure it's restored in the end.

Thanks to the finally cleaning up with store.f() we are sure this restoration happens correctly. In the manual unmamaged one we might have patterns where we don't which I am fine with due to this being a major bump.

TODO:

  • Add tests asserting that Signal.prototype._subscribe is restored to its initial form

@changeset-bot

changeset-bot Bot commented Nov 12, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7b11c8d

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

This PR includes changesets to release 2 packages
Name Type
@preact/signals-react Major
@preact/signals-react-transform Patch

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

@netlify

netlify Bot commented Nov 12, 2025

Copy link
Copy Markdown

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit 7b11c8d
🔍 Latest deploy log https://app.netlify.com/projects/preact-signals-demo/deploys/6a60c67bfc925000091f0056
😎 Deploy Preview https://deploy-preview-794--preact-signals-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions

github-actions Bot commented Nov 12, 2025

Copy link
Copy Markdown
Contributor

Size Change: +142 B (+0.07%)

Total Size: 190 kB

📦 View Changed
Filename Size Change
docs/dist/assets/bench-********.js 1.6 kB +1 B (+0.06%)
docs/dist/assets/client-********.js 46.7 kB +139 B (+0.3%)
docs/dist/assets/EmbeddedDevtools-********.js 17.8 kB +5 B (+0.03%)
docs/dist/assets/index-********.js 8.43 kB +1 B (+0.01%)
docs/dist/assets/signals-core.module-********.js 1.92 kB +2 B (+0.1%)
docs/dist/assets/signals.module-********.js 2.64 kB -3 B (-0.11%)
docs/dist/assets/Unmount-********.js 651 B -1 B (-0.15%)
docs/dist/assets/utils.module-********.js 514 B -2 B (-0.39%)
docs/dist/basic-********.js 246 B -4 B (-1.6%)
docs/dist/nesting-********.js 1.14 kB -3 B (-0.26%)
docs/dist/react-********.js 243 B +1 B (+0.41%)
packages/core/dist/signals-core.js 1.94 kB +1 B (+0.05%)
packages/core/dist/signals-core.mjs 1.93 kB +1 B (+0.05%)
packages/devtools-ui/dist/devtools-ui.js 16.3 kB +1 B (+0.01%)
packages/devtools-ui/dist/devtools-ui.mjs 15.7 kB +2 B (+0.01%)
packages/preact/dist/signals.mjs 1.74 kB +1 B (+0.06%)
ℹ️ View Unchanged
Filename Size
docs/dist/assets/devtools-********.js 912 B
docs/dist/assets/jsxRuntime.module-********.js 300 B
docs/dist/assets/preact.module-********.js 4.74 kB
docs/dist/assets/style-********.css 5.26 kB
packages/debug/dist/debug.js 4.64 kB
packages/debug/dist/debug.mjs 4.15 kB
packages/devtools-adapter/dist/devtools-adapter.js 2.36 kB
packages/devtools-adapter/dist/devtools-adapter.mjs 2.07 kB
packages/preact-transform/dist/signals-transform.js 1.69 kB
packages/preact-transform/dist/signals-transform.mjs 1.64 kB
packages/preact-transform/dist/signals-transform.umd.js 1.8 kB
packages/preact/dist/signals.js 1.81 kB
packages/react-transform/dist/signals-transform.js 7.65 kB
packages/react-transform/dist/signals-transform.mjs 6.8 kB
packages/react-transform/dist/signals-transform.umd.js 7.77 kB
packages/react/dist/signals.js 214 B
packages/react/dist/signals.mjs 165 B
packages/vite-plugin/dist/vite-plugin.js 8.86 kB
packages/vite-plugin/dist/vite-plugin.mjs 7.86 kB

compressed-size-action

Comment thread packages/react/runtime/src/index.ts Outdated

@andrewiggins andrewiggins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

signal1.value++;
signal2.value++;
});
expect(Signal.prototype._subscribe).to.equal(originalSubscribe);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I appreciate these checks to make sure the original implementation is restored

Assisted-By: devx/e0620a8d-8624-4748-b9c7-ce390238fc37
Assisted-By: devx/e0620a8d-8624-4748-b9c7-ce390238fc37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signals never unwatched when using hooks and suspenses

3 participants