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

React bindings sketch #72

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

ds300
Copy link

@ds300 ds300 commented Jun 18, 2024

Hi pals 👋🏼 This PR is a proof of concept to add React bindings for the signal-polyfill package, based on tldraw's internal fork of signia's react bindings.

Implementing sound react bindings required a couple of features that the spec doesn't have yet:

  1. Computed.isPending – to allow us to check whether a computed's source values have changed since it was last updated. This is needed because, much like the signal graph algorithm decides whether to recalculate a computed value based on whether its sources have changed, we need to decide whether to tell react to rerender based on whether the render function's reactive sources have changed without actually invoking the render function itself (since it might use react hooks, which throw when invoked outside of a react render cycle)
  2. Computed.get(forceRecalculate) – a version of Computed.get which ignores the cache. This is needed because you cannot memoize the result of a react render and simply return that result on future renders. The render may use hooks and React will throw errors if you don't invoke the same set of hooks on every render.

These features were added in a pnpm patch file. Unfortunately I had to patch the minified code but they're still quite readable changes.

"[email protected]": "patches/[email protected]"
}
},
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

Should these all be dev dependencies instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ye

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually -- should all of these move to the react sub-project? and not in the main utils package?
(I just saw that the main package has no changes)

@ds300 ds300 marked this pull request as draft June 18, 2024 14:37
@@ -9,6 +9,7 @@
"moduleResolution": "bundler",
"declaration": true,
"emitDeclarationOnly": true,
"jsx": "react-jsx",
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be better to set this in the react folder, so we don't force an interpretation of JSX to go a certain way

@@ -20,6 +21,10 @@ for (let entry of entryFiles) {
export default defineConfig({
// esbuild in vite does not support decorators
// https://github.com/evanw/esbuild/issues/104
test: {
// 👋 add the line below to add jsdom to vite
environment: 'jsdom',
Copy link
Collaborator

Choose a reason for hiding this comment

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

this library's tests run in node, firefox, and chrome -- so we may want to conditionally set this for node only

@littledan
Copy link
Contributor

The next step here will be to iterate on the underlying signals API, e.g., in the form of proposal-signals/signal-polyfill#22 or another variant that accomplishes the same thing.


expect(screen.getByText("count: 0")).toBeInTheDocument();

// it does not re-render when the signal is updated

Choose a reason for hiding this comment

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

Suggested change
// it does not re-render when the signal is updated
// it does re-render when the signal is updated

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.

5 participants