Skip to content

Commit

Permalink
[react-transform] Prepend useSignals call if function isn't clearly…
Browse files Browse the repository at this point in the history
… a component (#458)

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:
```js
/** @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.
  • Loading branch information
andrewiggins committed Nov 30, 2023
1 parent 03bb907 commit 0c0d89f
Show file tree
Hide file tree
Showing 4 changed files with 469 additions and 272 deletions.
5 changes: 5 additions & 0 deletions .changeset/little-donuts-notice.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@preact/signals-react-transform": minor
---

Only prepend useSignals call if we can't determine whether a function is a component or hook
6 changes: 5 additions & 1 deletion packages/react-transform/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,12 @@ function transformFunction(
// component. The try/finally in the component's body will stop tracking
// signals for us instead.
newFunction = prependUseSignals(t, path, state);
} else {
} else if (isComponentName(functionName)) {
newFunction = wrapInTryFinally(t, path, state);
} else {
// Since we can't determine if this function is a component/hook or not,
// we'll just prepend the useSignals call so it will work as either
newFunction = prependUseSignals(t, path, state);
}

// Using replaceWith keeps the existing leading comments already so
Expand Down

0 comments on commit 0c0d89f

Please sign in to comment.