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

Proposal for attribute+signal shorthand #85

Closed
Qix- opened this issue Apr 22, 2019 · 14 comments
Closed

Proposal for attribute+signal shorthand #85

Qix- opened this issue Apr 22, 2019 · 14 comments

Comments

@Qix-
Copy link
Contributor

Qix- commented Apr 22, 2019

Right now we can put signals in as attribute values as so:

const inputType = S.value('text');
const Foo = () => (<input type={inputType()} />);
inputType('password');

However would it be possible/better to allow direct usages of signals to be deduced and allowed?

const inputType = S.value('text');
const Foo = () => (<input type=inputType />);
inputType('password');

While fill=white traditionally worked with HTML, Surplus already (rightfully, IMO) throws as an error, so adding this wouldn't be breaking anyone. It'd simply be a nice shorthand.

Just a thought!

@derekhawker
Copy link

Looks a little confusing next to the syntax for passing the signal reference to be honest.

const Foo = () => (<input type={inputType} />);

It also wouldn't align with how Typescript typechecks JSX and being synced with Typescript should stay a priority IMO.

@Qix-
Copy link
Contributor Author

Qix- commented Apr 24, 2019

Does that work @derekhawker? I'm not near a computer to check but idk why it didn't occur to me to check that.

@derekhawker
Copy link

Not really in that case. That was a bad example. Here's a better one showing what I mean. I have a custom component Foo that expects a signal reference passed in the sig1 attribute.

function Foo ({ sig1 }){
    return <div>{sig1()}</div>;
}

const inputType = S.value('text');
<Foo sig1={inputType}/>

@ryansolid
Copy link

ryansolid commented Apr 25, 2019

Also I don't believe that syntax is valid JSX. So while a parser could be written to handle it, it could trip up any process that runs before that preserves JSX.

I tried to solve this previously without special syntax but the way some older libraries do this is to force you into their primitives so you can do an isObservable check at runtime but Surplus has no such limitations. It's a good thing. Everything is just a thunk. While not detectable it is more flexible and allows avoiding creating unnecessary computations.

At a certain point you have to decide if any extra syntax is better than 2 parentheses. I'm not sure it is.

Although solving this has been the motivation of atleast a few projects. Solid's use of proxies was originally trying to get away from those parens, but there are tradeoffs there and I ended up inventing my own parens based JSX syntax anyway.

I saw a library the other day called Fidan that actually compiles trailing $ (observable convention) into being treated as if they are observables so depending on the context of the access whether for writing or getting it produces the right code ie:

let value = data$.users$.name;
//compiles to
let value = data().users().name;

data$.color$ = 'red';
//compiles to
data().color('red');

Which is also interesting.

@Qix-
Copy link
Contributor Author

Qix- commented Apr 25, 2019

I guess the biggest push for this is the fact that I have to have special handling for signals vs. regular values.

If I have an <Image src={someValue} /> I have to introduce special handling in the image component to check if it's a signal or not in order to correctly handle strings and signals. It'd be great to be able to have something automatic there to clean up the code a bit.

@Qix-
Copy link
Contributor Author

Qix- commented May 28, 2019

Something worth mentioning: it's worth it to pass the signal itself as an attribute in a lot of cases (most cases, I'd say) since by doing <SomeComponent param={someSignal()} /> would re-create SomeComponent whenever someSignal changed, whereas <SomeComponent param={someSignal} /> would allow SomeComponent to selectively change pieces of itself - which is kind of the whole point.

However, this causes a bit of annoyance when the parameter could be a normal value, e.g. <SomeComponent param="Some String" /> - SomeComponent must now be aware of the fact that it could be passed a signal or a regular value.

What would be nice is a means to either refer to signals without the () or to indicate that a parameter should be converted to a signal if it is not.

That way, I don't need to hack together something like the following:

const sigval = v => (typeof v === 'function' && (v.name === 'data' || v.name === 'value')) ? v() : v;

The above is not a good way of handling it as any function with matching names would erroneously be called (and to my knowledge there is no other way of detecting that an arbitrary JS value is a signal - cc @adamhaile on that one).

This is the last piece of API-related frustration for me as I use Surplus more and more.

@ryansolid
Copy link

ryansolid commented May 28, 2019

I solved this with Solid a different way. Indicating on the binding that it was to be treated dynamic or not. Ie.. whether to wrap in a function or not. And then when the compiler identifies it is to be treated dynamic to wrap the accessor in an ES5 getter on the props object. That way the Component itself can always reference it as props.name and it may or may not be a signal. This was acceptable to me because Solid heavily uses proxies and since it is indicated by the compiler if you choose not to use this functionality a Component is still just a function in all other cases. This approach isn't congruent with Surplus at all though.

For Surplus to take this sort of tact in a way that makes sense for it, it would have to wrap Component props in functions which alway resolve the value. Basically turn everything into an accessor or Signal-like function.

const SomeComponent = (props) => {
    S(() => {
        // I'm always a function
        props.params();
        props.params2();
    });
    // .....
}
<SomeComponent param={someSignal()} params2="Some String" />

The problem of params being a Signal or a string from the caller goes beyond just binding the child. Interacting with that data at all would be unpredictable. If I'm trying to write a computed inside the Component, what is props.params? We can't assume the value always directly gets bound to some JSX. Is it really worth wrapping every component prop? Is it worth giving up a Component is just a function?

Or, should you consider a getter function the prop type. Params should always be a function and if you want to pass a string well:

<SomeComponent param={someSignal} params2={() => "Could be String"} params3="Always String" />

I don't know. Which is preferable? The former is much more consistent but has a performance cost. The latter is what we he today but you have to be conscious of Signals as Prop Types. I think that is reasonable for a library where Signals are first class citizens.

@Qix-
Copy link
Contributor Author

Qix- commented May 28, 2019

The difference, though, is that it would be reasonably trivial to add a marker to signals to indicate they are a signal. Even though they're pure functions, we should be able to add a symbol or something to indicate they're a signal and check for it.

What's important to note is that Surplus could bind the attribute upon element creation (based on the attribute's type). However, in order to avoid doing this for every attribute, there would have to be a special syntax to indicate it should occur.

Hence the OP. However, the deal breaker for the proposed syntax is that it would break any existing JSX parsers, e.g. typescript's, as @derekhawker pointed out.

@ryansolid
Copy link

The marker at runtime would limit potential use and cause unnecessary Reactive graph node creation, where just using functions reduces that overhead. You should never bank on knowing the function being passed in contains a signal or not. I actually asked @adamhaile for this functionality isSignal at one point but realized he was right. It doesn't matter if a function is a Signal but rather it contains a Signal resolution somewhere in its execution. Older libraries always wrapped expressions, but part of Surplus' performance comes from not doing so. I know Adam has been working on ways with S to lower the cost of inert Computation Nodes that is unreleased so maybe potential here in the future, but depending isSignal would put big restrictions on what you can do.

That being said at compile time is possible(did so with Solid) I'm just unsure a really clean solution for Surplus. I mean I'm sure it's possible to come up with a non-breaking clean syntax. I'm just unclear what an ideal behavior that is consistent across native elements and Components. Perhaps it just needs more thought. I feel a lot of this has been talked about in other issues. But perhaps @adamhaile can throw in where he is on this one.

@adamhaile
Copy link
Owner

Thanks, Ryan, for fielding this one and proxying our former conversations :).

Yeah, two important points here are:

  1. preserving JSX compat unless we have a very good reason not to.
  2. signals in S are just functions. Not a subset of functions -- any paramless function is a valid signal in Surplus. This enables lots of composability and a usually a cleaner syntax.

On #2, I'd have to see a very good case for why a consumer should care whether the incoming function is a signal or not to add an S.isSignal() method. Maybe I'm missing one, so feel free to chime in if you have an example :).

I'll say that, in general, thinking about which pieces of data can change and where that change is handled is usually an important part of building Surplus components. Component signatures that are overloaded -- take either a string or a function producing a string -- can make this confusing and hard to track. But that's in my experience, so I'm happy to hear others' opinions. This is also why I, personally, prefer explicit first-class signals rather than proxies (pace @ryansolid :) ), though I understand others feel differently. The fact that I use typescript for most projects may be an influence here: it's a little extra friction to type properties as param : string | () => string all the time rather than just param : () => string. My recent work on S is to open up the core to more binding strategies without making those approaches second-class citizens to S's default function-based signals.

I'm willing to be pragmatic, though. The one exception to "no overloading" is in insertions -- <div>{code}</div> -- where code can evaluate either to the value being inserted or to a function producing such. That was to handle in-line usage of utilities that transform signals -> signals, like SArray's .map() method.

@Qix-
Copy link
Contributor Author

Qix- commented Jun 18, 2019

Yeah I mean the spirit of the original issue is that having to handle cases where the incoming attribute values can be either a value or a function returning a value is quite cumbersome.

I current have a utility that does this for me, which I hate having to use:

export const sig = v => typeof v === 'function' ? v : () => v;

Which I use like this:

import {sig} from '../util';

export const MyComponent = ({value}) => {
    value = sig(value);

    return (
        <div>{value()}</div>
    );
};

in order to support

<MyComponent value={1234} />

const someValue = S.data(5678);
<MyComponent value={someValue} />

Yes, I could do

const someValue = S.data(5678);
<MyComponent value={someValue()} />

But that causes the MyComponent element to be re-created entirely whenever someValue changes.

I fully 100% agree with the composition argument, and I do agree that S.isSignal is poor form.

It's just the syntax that gets hairy, that's all.

@adamhaile
Copy link
Owner

FWIW, the utility I've usually seen used for this is sort of the opposite of your sig and makes for slightly cleaner syntax:

export const unwrap = v => typeof v === 'function' ? v() : v;
import { unwrap } from '../util';
export const MyComponent = ({value}) => 
        <div>{unwrap(value)}</div>;

De-ref happens inside {...} so it's still fine-grained.

@Qix-
Copy link
Contributor Author

Qix- commented Jun 19, 2019

Hmmm yeah okay, that is indeed better.

I'm curious to see what kinds of things your binding upgrades will let me do, too :)

@Qix-
Copy link
Contributor Author

Qix- commented Oct 11, 2019

Closing this, thanks for the insightful conversation!

@Qix- Qix- closed this as completed Oct 11, 2019
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

No branches or pull requests

4 participants