opral / monorepo

lix (change control system) && inlang (globalization ecosystem for software built on lix)
https://opral.com
Apache License 2.0
1.19k stars 100 forks source link

inlang project: async function calls should be async #1290

Closed samuelstroschein closed 1 year ago

samuelstroschein commented 1 year ago

Discussed in https://github.com/inlang/inlang/discussions/1272

Originally posted by **samuelstroschein** August 22, 2023 ## Problem - treating async operations sync will lead to bugs in apps - awaiting async operations with a dedicated `init()` is unergonomic The reactive system treats async operations like `inlang.lint.reports` as sync operations leading to the requirement to call `await inlang.lint.init()` before accessing lint reports. Awaiting `init` is arguably inconvenient and will definitely lead to "gotcha" moments. More importantly though, awaiting `init` will lead to bugs. #### Example The production code below struggles with awaiting an update operation and introduces a workaround by awaiting a promise that will eventually resolve. A gateway to bugs by thinking "this is sync, why is my message not immediately update?", forgetting to establish such a workaround, maybe the timeout is too fast, etc. https://github.com/inlang/inlang/blob/3c4b975a47c427dbb8cbdf565eedca0a75cdd0da/source-code/core/app/src/wrappers/withSolidReactivity.test.ts#L259-L261

Sub-tasks

samuelstroschein commented 1 year ago

@janfjohannes do you want to implement an async primitive while you factor out lix/reactive, see

_https://github.com/inlang/inlang/pull/1142#discussion_r1305526884_

@janfjohannes why does lix depent on inlang/app?

If we want to share reactive primitives, lets extract the reactive primitives and adapters from inlang/app to lix/reactive-primitives or something.

The logical deduction is that "inlang is a lix app". Not the other way around "lix is an inlang app"

janfjohannes commented 1 year ago

@samuelstroschein i did a first version using the same solid to subscribable method as in inlang/app, there were stil too many open questions to do somthing propper and did not want to block @NilsJacobsen any more.

i think we need a proper working session to make a good plan as you said, this should play nicely with awaitng and async. i really currently fancy something using async iterables as this allows awaiting and has native js support but i am not 100% sure about this.

samuelstroschein commented 1 year ago

@janfjohannes we should ask in the solid discord what they recommend/what's upcoming in solidjs 2.0. i heard that solid 2.0 has first class async support. coordinate with @ivanhofer pls. it's clear that we need sync and async reactivity for both inlang and lix/client

janfjohannes commented 1 year ago

@samuelstroschein i checked in the rxjs froums and they have solved it in rxjs 7 like this, and they also have a subscribable:

// these are awaitable 
await rxjs.lastValueFrom(inlang.someapi.something)
await rxjs.firstValueFrom(inlang.someapi.something)

however as in a reactive system the subscribable never completes until the reactivity is disposed we probably need somthing liek await nextValueFrom(inlangApiObservable) wher the promise would resolve when the next value is produced after the subscription is started

janfjohannes commented 1 year ago

@ivanhofer you know solid much better than me and also had interactions with the team before, could you get feedback for a best practice from solids perspective?

samuelstroschein commented 1 year ago

@janfjohannes the solid community is friendly. I would ask in the #reactivity channel. Say that you come from inlang.

PS we are in elaborating a sponsorship for solidjs. If solid (2.0) suits out use case for lix and inlang reactivity best, we sponsor them for ongoing support/use inlang to build their i18n stuff

samuelstroschein commented 1 year ago

It seems to me that we need an asyncSignal primitive. Solid offers resource maybe that's what ne need.

signal()
asyncSignal()
derived()
effect()
NilsJacobsen commented 1 year ago

I'm gonna make some tryouts today. Will discuss the outcome with you later.

samuelstroschein commented 1 year ago

@NilsJacobsen

The API I have in mind is

const lint = asyncSignal(() => lintMessages({}))

await lint()

Questions:

  1. How to lint all messages and then only changed messages? I guess that needs to be handled in lintMessages internally
NilsJacobsen commented 1 year ago

Looks good, Under the hood, we also want lint to be an async subscibable right? And then with the adapter an async signal. I'm gonna start experimenting with lint.

samuelstroschein commented 1 year ago

Looks good, Under the hood, we also want lint to be an async subscibable right? And then with the adapter an async signal.

Yes, an async subscribable entails that we need an async signal.

NilsJacobsen commented 1 year ago

Okey what I have right now is that createAsyncSubscribable

export function createAsyncSubscribable<T>(
    signal: () => T,
    init: () => Promise<void>,
): AsyncSubscribable<T> {
    const asyncSignalGetter = async () => {
        await init()
        return signal()
    }
    return Object.assign(asyncSignalGetter, {
        subscribe: (callback: (value: T) => void) => {
            createEffect(() => {
                callback(signal())
            })
        },
    })
} 

leads to this usage:

await inlang.lint() //get value
inlang.lint.subscribe((m) => console.log(m)) // access callback

await inlang.lint() // TODO: get signal, does not work right now

The whole topic is getting more and more complex. Maybe let's discuss tomorrow morning how we can make it work this week.

samuelstroschein commented 1 year ago

We have a hacky implementation in place. Future update will follow