preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.64k stars 89 forks source link

[react-transform] Prepend `useSignals` call if function isn't clearly a component #458

Closed andrewiggins closed 7 months ago

andrewiggins commented 7 months ago

It's possible to use the @trackSignals comment manually opt-in to the react signals transform on a function that isn't obviously a Component or hook. Before, we would always wrap those functions in a try/finally clause. However, this could break signal tracking if the function was actually used as a hook.

For example:

/** @trackSignals */
function badHookName() {
  try {
     // When this hook runs it will close any open effect. Specifically,
     // the effect in Component. This means, upon exiting this hook,
     // all effects will be closed and any signals accesed in "... other stuff ..."
     // in Component will be missed.
     const e = useSignals();
     ...
   } finally {
     e.f() // Finish this hook's effect
   }
}

function Component() {
  try {
    const e = useSignals();
    badHookName();
    // .. other stuff ...
  } finally {
    e.f(); // Finish this hook's effect.
  }
|

We fix the bug described in the comment above by just prepending useSignals(). This method of tracking signals in React relies on a microtick to close the effect meaning any signals accessed in "... other stuff ..." will be captured by this effect.

More improvements to this scenario will come in a follow up PR.

This PR also adds more tests for transforming hooks in general.

changeset-bot[bot] commented 7 months ago

πŸ¦‹ Changeset detected

Latest commit: fe0ff1d4dd9c67196eda0542b853ed727c6ac334

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

This PR includes changesets to release 1 package | Name | Type | | ------------------------------- | ----- | | @preact/signals-react-transform | Minor |

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[bot] commented 7 months ago

Deploy Preview for preact-signals-demo ready!

Name Link
Latest commit fe0ff1d4dd9c67196eda0542b853ed727c6ac334
Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/6568583d4c870000086b74d0
Deploy Preview https://deploy-preview-458--preact-signals-demo.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

github-actions[bot] commented 7 months ago

Size Change: +202 B (0%)

Total Size: 84.1 kB

Filename Size Change
packages/react-transform/dist/signals-*********.js 4.83 kB +67 B (+1%)
packages/react-transform/dist/signals-transform.mjs 4.08 kB +68 B (+2%)
packages/react-transform/dist/signals-transform.umd.js 4.95 kB +67 B (+1%)
ℹ️ View Unchanged | Filename | Size | | :--- | :---: | | `docs/dist/assets/client.********.js` | 46.8 kB | | `docs/dist/assets/index.********.js` | 1.07 kB | | `docs/dist/assets/jsxRuntime.module.********.js` | 281 B | | `docs/dist/assets/preact.module.********.js` | 4.02 kB | | `docs/dist/assets/signals-core.module.********.js` | 1.46 kB | | `docs/dist/assets/signals.module.********.js` | 2.02 kB | | `docs/dist/assets/style.********.js` | 21 B | | `docs/dist/assets/style.********.css` | 1.21 kB | | `docs/dist/basic-********.js` | 244 B | | `docs/dist/demos-********.js` | 3.41 kB | | `docs/dist/nesting-********.js` | 1.13 kB | | `docs/dist/react-********.js` | 240 B | | `packages/core/dist/signals-core.js` | 1.54 kB | | `packages/core/dist/signals-core.mjs` | 1.56 kB | | `packages/preact/dist/signals.js` | 1.27 kB | | `packages/preact/dist/signals.mjs` | 1.22 kB | | `packages/react/dist/signals.js` | 1.43 kB | | `packages/react/dist/signals.mjs` | 1.4 kB |

compressed-size-action