Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add package @preact/signals-react-transform #375

Merged
merged 44 commits into from
Aug 2, 2023
Merged

Conversation

andrewiggins
Copy link
Member

@andrewiggins andrewiggins commented Jun 23, 2023

This PR adds a new package @preact/signals-react-transform that provides a Babel plugin to transform functions to observer/track what signals are used when the function runs. This plugin is the start to an alternate React integration for signals that does not hook into React internals. Once our approach here is finalized and published, I'll update the @preact/signals-react package to reference this and provide various options for using signals in React.

Check the test file for examples of what the transformation looks like.

I'm currently exploring two different kinds of transformations in this implementation to see which one works better. One just adds a call to useSignals() to the top of the component and relies on the next invocation of useSignals or a microtick to stop tracking signals. The other wraps the component body in a try/finally to start and stop tracking signals. I'm currently leaning towards making the try/finally the default cuz it leaves less room for tricky edge cases that may lead to non-obvious behavior and rerenders.

However, the implementation of useSignals is the same either way. We'll always setup the microtick to clean up any outstanding effects to prevent any loose ends regardless of which transformation is used. So users can manually drop useSignals into a component and get basic functionality.

@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: 255208a

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 Patch
@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
Copy link

netlify bot commented Jun 23, 2023

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit 255208a
🔍 Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/64cad3e1ec66ea00087a05d6
😎 Deploy Preview https://deploy-preview-375--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
Copy link
Contributor

github-actions bot commented Jun 23, 2023

Size Change: +267 B (0%)

Total Size: 78 kB

Filename Size Change
docs/dist/assets/client.********.js 46.7 kB +95 B (0%)
docs/dist/react-********.js 238 B -1 B (0%)
packages/react/dist/signals.js 1.35 kB +89 B (+7%) 🔍
packages/react/dist/signals.mjs 1.29 kB +84 B (+7%) 🔍
ℹ️ View Unchanged
Filename Size
docs/dist/assets/index.********.js 835 B
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.43 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.35 kB
docs/dist/nesting-********.js 1.13 kB
packages/core/dist/signals-core.js 1.5 kB
packages/core/dist/signals-core.mjs 1.53 kB
packages/preact/dist/signals.js 1.27 kB
packages/preact/dist/signals.mjs 1.22 kB
packages/react-transform/dist/signals-*********.js 2.79 kB
packages/react-transform/dist/signals-transform.mjs 2.66 kB
packages/react-transform/dist/signals-transform.umd.js 2.91 kB

compressed-size-action

karma.conf.js Outdated
@@ -283,7 +287,7 @@ module.exports = function (config) {
jsx: "preserve",

// esbuild options
target: downlevel ? "es5" : "es2015",
target: downlevel ? "es5" : "es2019",
Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops - will undo this. Did this to not downlevel async/await while debugging tests

@@ -0,0 +1,119 @@
# Signals React Transform
Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure what's the right level of detail here. Would love other's thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's perfectly fine. We could theoretically shorten the intro paragraph about what signals are by just pointing to the blog post. But then again that I think leaving as is, is fine too.

Comment on lines +11 to +13
// TODO:
// - how to trigger rerenders on attributes change if transform never sees
// `.value`?
Copy link
Member Author

Choose a reason for hiding this comment

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

I still need to build the signal attribute handling in our jsx handler

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this in a follow up PR. this one is big enough 😅

Comment on lines +210 to +214
// TODO: Consider alternate implementation, where on enter of a function
// expression, we run our own manual scan the AST to determine if the
// function uses signals and is a component. This manual scan once upon
// seeing a function would probably be faster than running an entire
// babel pass with plugins on components twice.
Copy link
Member Author

Choose a reason for hiding this comment

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

For anyone with more experience with babel transforms - if I want to know if a function body contains JSX to know whether to perform my transformation, is it better to do a local visitor pass inside this ArrowFunctionExpression handler to observe that (only running my local visitor), and then transform? Or to do the observation in the larger babel visitor pass and then transform after, which triggers and additional visitor pass with all plugins?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I've never thought about this.

Copy link
Member

Choose a reason for hiding this comment

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

You can use something like this on a VariableDeclaration to see whether they are used as JSX

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh interesting. IIUC, this code looks to see if a particular identifier is used as JSX somewhere in the same file?

@andrewiggins
Copy link
Member Author

andrewiggins commented Jun 29, 2023

TODO for myself: Add some E2E babel transform tests, i.e. transform code and run it. Done

@XantreDev
Copy link
Contributor

What if someone calls useSignals twice? Or babel will add additional one?

@andrewiggins
Copy link
Member Author

andrewiggins commented Jul 12, 2023

What if someone calls useSignals twice? Or babel will add additional one?

useSignals can be called multiple times in a component. It'll recreate the effect and other relevant tracking machinery but that's okay. The previous effect will be cleaned up and any signals used after the additional calls to useSignals will still trigger rerenders properly.

What about cases when signals used inside hooks that component uses? How to handle this case?

Hooks that use signals are transformed to call useSignals at the top of the function. In this situation, we are relying on the fact that calling useSignals multiple times is safe to do.

Copy link
Member Author

@andrewiggins andrewiggins left a comment

Choose a reason for hiding this comment

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

Okay! I believe this is now ready for review 🎉

Comment on lines +11 to +13
// TODO:
// - how to trigger rerenders on attributes change if transform never sees
// `.value`?
Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this in a follow up PR. this one is big enough 😅

return hasLeadingComment(path, optOutCommentIdentifier);
}

function isOptedIntoSignalTracking(path: NodePath | null): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here and in isOptedOutOfSignalTracking, I'm climbing up the AST to find the most logical place for a comment to live. For example supporting a use case such as:

/** @trackSignals */
export default function useCustomHook() {}

@andrewiggins andrewiggins marked this pull request as ready for review July 12, 2023 22:09
return (
fnNameStartsWithCapital(path) && // Function name indicates it's a component
getData(path.scope, containsJSX) === true && // Function contains JSX
path.scope.parent === path.scope.getProgramParent() // Function is top-level
Copy link
Contributor

Choose a reason for hiding this comment

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

What about high order components? This type of components can use signals too

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, it's true. I'm thinking HoC will need to manually opt-in using /** @trackSignals */ or by calling useSignals() directly in their wrapper function.

It is harder to detect HoCs and know where to put the useSignals call so for now I'm explicitly skipping them. I should add a note to the Readme calling this out for people who want to use signals in higher order components.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it a good idea to mention edge cases in readme

signal.value;
return <div>Hello World</div>;
} finally {
_effect.f();
Copy link
Member Author

Choose a reason for hiding this comment

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

Any opinions on the name of this method f? It's short for finishEffect to stop the current effect.

I've changed the return of useSignals from an method to an object that can hang other stuff of like the dispose symbol for using with using which this PR also includes support for.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This looks amazing

Comment on lines +210 to +214
// TODO: Consider alternate implementation, where on enter of a function
// expression, we run our own manual scan the AST to determine if the
// function uses signals and is a component. This manual scan once upon
// seeing a function would probably be faster than running an entire
// babel pass with plugins on components twice.
Copy link
Member

Choose a reason for hiding this comment

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

You can use something like this on a VariableDeclaration to see whether they are used as JSX

@andrewiggins andrewiggins mentioned this pull request Jul 20, 2023
@andrewiggins andrewiggins merged commit fe44f6e into main Aug 2, 2023
7 checks passed
@andrewiggins andrewiggins deleted the react-babel-plugin branch August 2, 2023 22:23
@github-actions github-actions bot mentioned this pull request Jul 31, 2023
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.

None yet

4 participants