WebReflection / usignal

A blend of @preact/signals-core and solid-js basic reactivity API
ISC License
220 stars 15 forks source link

reconsider .then() #28

Closed nuxodin closed 1 year ago

nuxodin commented 1 year ago

Hi Andrea

However much I like "then()", here it can lead to problems. Or am I missing a way to return a signal from an asynchronous function?


async function getSignal(){
  cons value = await fetchValue();
  return signal(value)
}

const s4 = await getSignal();

console.log(s4.value) // undefined
console.log(s4) // the value
WebReflection commented 1 year ago

I don't understand the use case for this ... that async function is not an effect, what are you after there? then() is meant to return signals containing promises in them, not the other way around. Closing until it's clear what you are doing.

signal(fetch('url')); // 👌
fetch('url').then(v => signal(v)); // 🤷
nuxodin commented 1 year ago

This is perhaps an understandable use case.

async function insert(name) {
    const lastInsertId = await query(`INSERT INTO x SET name = ${name}`);
    const nameSignal = signal(name);
    effect(()=>{
        query(`UPDATE x SET name=${nameSignal} WHERE id=${lastInsertId}`);
    })
    return nameSignal;
}

signal = await insert('Joe'); // should await for insert, but return a signal
signal.value = 'John';

I mean no async function is able to return a signal.

WebReflection commented 1 year ago
query(`INSERT INTO x SET name = ${name}`);

either there's an error there, or that query fails and it's shenanigans prone security wise, same goes for the query inside the effect.

That being said, a signal can contain a Promise, primary use case, so this one would be different, as you know the name to insert already:

async function insert(signal) {
    const lastInsertId = await query(`INSERT INTO x SET name = ${signal.peek()}`);
    effect(()=>{
        query(`UPDATE x SET name=${signal} WHERE id=${lastInsertId}`);
    })
    return signal;
}

let name = await insert(signal('Joe')); // should await for insert, but return a signal
name.value = 'John';
WebReflection commented 1 year ago

OK, maybe I got the issue there ... the returned signal resolves ... bummer, but both cases are valid to me, and used already.

How about this?

async function insert(signal) {
    const lastInsertId = await query(`INSERT INTO x SET name = ${signal.peek()}`);
    effect(()=>{
        query(`UPDATE x SET name=${signal} WHERE id=${lastInsertId}`);
    })
    return {
      get value() { return signal.value },
      set value(_) { signal.value = _ }
    };
}

???

not the best answer, but the first that comes to mind ... the current signature is kinda sailed, unless I release a major with reasonable good enough reasons and use cases why current feature is not welcome ... arguably, the current signature indeed doesn't fully allow a value setter, so I need to think about it!

WebReflection commented 1 year ago

P.S. what's Preact doing in here? happy to hear from their experience around this topic too

nuxodin commented 1 year ago

either there's an error there...

This is just minimal dummy code, but maybe I should write it properly for ChatGPT 🤣

P.S. what's Preact doing in here? happy to hear from their experience around this topic too

It seams Preact-Signals are not thenables

WebReflection commented 1 year ago

it's possible I made them so due computed, see https://github.com/WebReflection/usignal/blob/main/test/test.js#L16-L21

I am thinking effectively signal(fetch('url')) makes very little sense, as that's a read-only signal in meaning and intent ... but computed(() => signal.value + fetch('url')) which is indeed read only, makes sense as awaitable, although that could be inferred by computed(async () => stuff) which should already work out of the box ... OK, I am fairly convinced now this was a mistake on my side: do you have any other use case to think about? should then just disappear as helper as it makes any signal effectively read-only ?

WebReflection commented 1 year ago

although that could be inferred by computed(async () => stuff) which should already work out of the box

it doesn't, as computed won't resolve at distance async callbacks ... once again, what's Preact position in here? 🤔 (guessing computed are not thenable neither ... yet it's an interesting after-thought to me)

nuxodin commented 1 year ago

I am fairly new to signals and have never come across a usecase for await signal. But if the benefit is only the saved ".value", I would do without it.

WebReflection commented 1 year ago

that was the overwhelming poll result: https://twitter.com/WebReflection/status/1571400086902476801

admittedly, neither me nor others presented use cases for such scenario, so it was "community driven" and it looks like the community didn't think enough about it 😅

nuxodin commented 1 year ago

I was also for "yup" 😬

WebReflection commented 1 year ago

cool, so it's your fault ... 🤣 what are you suggesting? dropping the then seems to fix them all and it's a breaking change that should go major (I don't even remember if this is major already) ... alternatives to then ??? I can't see myself any

WebReflection commented 1 year ago

now that I've looked at the history here ... you helped improving then there ... do you you remember why you did so? if it was before realizing it was a mistake, then I'm OK dropping that, specially as we all agree at this point this was my mistake https://twitter.com/_developit/status/1628868903404290049

nuxodin commented 1 year ago

Yes, I am new to signals and I have only just noticed this problem.

You could maybe leave the function (then) in for the next version and output a console.error('deprecated') to draw attention to the change.

WebReflection commented 1 year ago

but I want to be able to do your example now 😁

WebReflection commented 1 year ago

I've started major updates to side/smaller libraries but I'll drop this thenability nonsense in here too.

WebReflection commented 1 year ago

OK, v0.9 drop .then(...) all together and it breaks whoever was using it before:

https://github.com/WebReflection/usignal/commit/73c6abf37162742fc2326a0817a48b4bd8428dab

this is a young module and I think .then wasn't too popular so I think it's OK this way and being semver compat I don't even feel bad for it 😇