-
-
Notifications
You must be signed in to change notification settings - Fork 116
feat: add jsxBind
function to @preact/signals
#724
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 00ad633 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This is missing a changeset btw |
Co-authored-by: Jovi De Croock <[email protected]>
Co-authored-by: Jovi De Croock <[email protected]>
if ( | ||
value && | ||
typeof value === "object" && | ||
Object.getPrototypeOf(value) === jsxBind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just using .prototype will be lower bytes and more performant I think, that being said not too fuzzed can look at this later. Will do one final pass over the weekend and get this merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that wouldn't be equivalent. it's either getPrototypeOf or .__proto__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using constructor would be cheaper and smaller here.
if (typeof value === 'object' && value.constructor === jsxBind) {
// ...
}
// ...
export function jsxBind<T>(cb: () => T): T {
return { value: cb, constructor: jsxBind } as any;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@developit The prototype needs to inherit from Signal
(see line below jsxBind declaration) for the SignalValue
component to be used (jsxBind(…)
expression as JSX child).
personally, I don't see an issue with using __proto__
as I did originally, but I digress
I'd like to think about this for a few more days before we commit to landing it. I don't think the name is quite right - this isn't binding, it's deriving or computing. Also, I keep thinking it would be better if we just built this in so it didn't require an import, simply by checking for a special character at the end of the prop name in our existing loop: <div class$={() => `foo ${sig.value}`} /> Alternatively, what if we just checked the type of the prop value: <div class={() => `foo ${sig.value}`} /> The only prop values that currently commonly accept functions in Preact are click handlers. We could have the logic be this: let props = vnode.props;
for (let i in props) {
if (i === "children") continue;
let value = props[i];
if (typeof value === 'function' && i[0] !== 'o' && i[1] !== 'n') {
value = oldSignalProps?.[i] || computed(value);
} |
Also I believe there is a slight issue with the implementation here (and my suggestions above). In the following example, re-rendering let counter = 0;
let someSignal = signal('hello');
function Demo(props) {
const active = useSignal(false);
return <div class={jsxBind(() => `${props.class}${active.value ? ' active' : ''}`)} />
} |
@developit As I said in the linked Slack thread, I would also prefer not needing the
That's intentional. It mimics the behavior of |
heh yeah I was just going to point out that this is the same as computed/useComputed. BTW - does this come up because you're trying to write components that never re-render? If so, DM me. I ... have been working on a thing. haha. |
Background
I briefly discussed this idea with @rschristian, who said I should try to implement it.
Link to discussion: https://preact.slack.com/archives/C3NMBSJKH/p1752172452437339
Goal
Avoid needing to declare "computed expressions" with
useComputed
, enabling you to write "inline computeds" where they're needed. This is intended for computeds that are only used by one JSX attribute or JSX child.How It Works
It's as easy as wrapping a "computed callback" with
jsxBind(…)
, a function exported by the@preact/signals
package.Key Features
useComputed
, the callback does not update on rerenderCurrent Limitations
Currently, there is no warning when trying to use
jsxBind
with a composite element, as this would require iterating the props object. Maybe someone has an idea about how to efficiently check for such misuse.Naming
Originally, I named it
bind()
, but decidedjsxBind()
makes its purpose a bit clearer. Ultimately, the name is up to you guys. :)