monorepo icon indicating copy to clipboard operation
monorepo copied to clipboard

not leak reactivity to consumer

Open samuelstroschein opened this issue 1 year ago • 14 comments

Problem

The current inlang/SDK and lix/client API shouldn't leak reactivity to the consumer.

  • we don't know which reactivity pattern will become the standard
  • (heavy) leakage of a reactivity pattern increases adoption friction because the reactivity pattern must be explained
// this should work
console.log(project.errors)

// but this is currently required
project.errors.subscribe((e) => console.log(e))

Proposal

  1. Expose a standard JS object similar to the window.* API.
  2. (Optional) wrappers that take the proxy object and adapt it to the environment the SDK is running in

pros

  • de-risk our API decision.
  • lower friction when adopting inlang and/or lix

cons

  • more explanations needed because adapters need to be explained

1. plain JS object with no need for reactivity(a CLI for example)

const repo = await openRepository()
const project = await loadProject()

console.log(repo.errors)
console.log(project.errors)

2. an env that needs to react to changes (GUI)

const repo = await withReactAdapter(openRepository())
const project = await withReactAdapter(loadProject())

function Component() {
  const hasErrors = repo.errors.length > 0
  return <>
    {hasErrors ? <p>please reload the repo</p> : null}
    <p>current branch is {repo.currentBranch}</p>
  </>
}

Open questions

~How to deal with setters?~

EDIT: They must be function calls because most setters are async

JavaScript setters use =. Most reactive systems have an explicit setter.

// this will trigger a side effect
window.location.href = "/de/hallo"

// should we adopt this pattern for the SDKs too?
repo.branch = "fluffy-gummibear"

// instead of
repo.setBranch("fluffy-gummibear")

samuelstroschein avatar Jan 31 '24 20:01 samuelstroschein

agree we should not leak reactivity details but not sure why a proxy is required or the right way to do this.

janfjohannes avatar Feb 01 '24 14:02 janfjohannes

agree we should leak reactivity details but not sure why a proxy is required or the right way to do this.

a proxy might not be needed. getter functions like the window.* API could also work https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get?retiredLocale=de. in any case, the API access needs to be "fake static" e.g. project.errors and not project.errors()

samuelstroschein avatar Feb 01 '24 15:02 samuelstroschein

@samuelstroschein @jldec and me also had a quick talk about this issue. we are still not sure if it might make more sense to expose the functional core for non reactive usecases and then the framework integration would expose the reactive api with whatever the framework has as idiomatic api format. but as close to the core api as possible. in this scenario we would not have to worry about adapting the reactive api layer back to normal looking js

janfjohannes avatar Feb 06 '24 17:02 janfjohannes

@samuelstroschein i assigned me too, as i am about to evaluate this from lix perspective next week.

janfjohannes avatar Feb 11 '24 16:02 janfjohannes

@samuelstroschein i think i would prefer an api wihtout having to manually wrap the original objects eg.

import { openRepo } from "@lix-js/svelte-client"

const repo = openRepo(....)

isntead of import { openRepo } from "@lix-js/client" import { asSvelteStore } from "@lix-js/svelte-client"

const repo = asSvelteStore(openRepo(....))

there is no usecase to pass something else into the adapter and we still do not leak any code from the adapeters into the bare libraries. what do you think?

janfjohannes avatar Feb 11 '24 16:02 janfjohannes

the main open question is how we deal with synchronous vs async data.

eg. its easy to expose branchname as a property as its always available

$: console.log(repo.branch)
>> "main"

but how do we handle data that is costly to generate/fetch and costly keep updated eg. forkStatus/ all available branches?

there are a few options (also some i do not think of right now, lets update this conversation when we think of more):

const status = await repo.forkStatus
// (or forkStatus()   i ignore function vs property api for now to focus on the async aspect)
// (would need some wrapper  like onMount() in most frameworks as high level await is mostly not possible)
  1. (could be on a repo level or on level of individual properties)
const status = repo.forkhStatus   
$: console.log($status)

 // is initially empty as data is only available async, exact api is out of scope for discussion, but loading state could be exposed as shown
//>> initially logs { loading: true} 
//>> after status is available store updates and logs  eg.{ ahead: 1, behind: 2, conflicts: false }

btw in my state management library atreyu i expose a proxy store that works in both ways but has maybe a bit awkward api :


$: console.log($repo.forkStatus$)   > behaves as example 2

$repo.forkStatus$promise > behaves like example 1

janfjohannes avatar Feb 11 '24 16:02 janfjohannes

i think i would prefer an api wihtout having to manually wrap the original objects eg.

Too risky as a day 1 choice.

We don't know how the requirements unfold. I would keep as much (all?) framework specific stuff out of lix/provide it with adapters. For example, web project written in metaframeworks will need access to plain JS repo./project. APIs on the server and a wrapped version on the client in the same codebase. The wrapping approach would allow this.

const project = await openRepo()
const projectClientSide = svelteAdapter(project)

Is this code above the best DX we will be able to deliver? Probably not. We can easily offer additions in the future.

samuelstroschein avatar Feb 11 '24 16:02 samuelstroschein

Btw I think this discussion can be closed. Agree @janfjohannes ?

  • we expose getter functions
  • we can't expose static props due to the async nature
  • hence, the current API is "correct"

samuelstroschein avatar Feb 11 '24 16:02 samuelstroschein

i think i would prefer an api wihtout having to manually wrap the original objects eg.

Too risky as a day 1 choice.

We don't know how the requirements unfold. I would keep as much (all?) framework specific stuff out of lix/provide it with adapters. For example, web project written in metaframeworks will need access to plain JS repo./project. APIs on the server and a wrapped version on the client in the same codebase. The wrapping approach would allow this.

const project = await openRepo()
const projectClientSide = svelteAdapter(project)

Is this code above the best DX we will be able to deliver? Probably not. We can easily offer additions in the future.

does not work as the adapter needs control about when repo is opened and how the async stuff is eexecuted.

the first argument i also dont understand you can import lix core or the framework adapter many frameworks work that way.

janfjohannes avatar Feb 12 '24 23:02 janfjohannes

Btw I think this discussion can be closed. Agree @janfjohannes ?

* we expose getter functions

* we can't expose static props due to the async nature

* hence, the current API is "correct"

dont think i could answer or finally agree / disagree with any of these without going through my tests. i would def. leave this open.

janfjohannes avatar Feb 12 '24 23:02 janfjohannes

the first argument i also dont understand you can import lix core or the framework adapter many frameworks work that way.

We will have an easier time communicating communicating "an adapter hooks into a loaded project/repo" than "the inlang/lix SDK exposes multiple load project/repo functions for your given environment".

Adapter hooking into a project

import { loadProject, svelteAdapter } from "@inlang/sdk"

const project = await loadProject()
const projectClientSide = svelteAdapter(project)

Separate load project/repo functions

import { loadProject } from "@inlang/sdk"
import { loadProject as loadProjectSvelte } from "@inlang/sdk/svelte"
// or import { loadProjectSvelte } from "@inlang/sdk" 

const project = await loadProject()
const clientSideProject = await loadProjectSvelte()

does not work as the adapter needs control about when repo is opened and how the async stuff is executed.

Please elaborate why the adapter hooking into an existing project pattern would not work.

The current (and likely future API) of exposing some form of a subscribable seems all we need to build adapters. I don't understand why an adapter needs to influence internal computations of the inlang/lix SDK. Mapping the exposed subscribable/observable/whatever to a frameworks reactivity system seems to be the only concern of an adapter, no?

samuelstroschein avatar Feb 13 '24 01:02 samuelstroschein

Please elaborate why the adapter hooking into an existing project pattern would not work.

The current (and likely future API) of exposing some form of a subscribable seems all we need to build adapters. I don't understand why an adapter needs to influence internal computations of the inlang/lix SDK. Mapping the exposed subscribable/observable/whatever to a frameworks reactivity system seems to be the only concern of an adapter, no?

because ui client libraries have to be reactive and synchronous for most frameworks but openRepo is async. this would not be possible because it has an await

const project = await openRepo()
const projectClientSide = svelteAdapter(project)

this would be the expected client sdk

const repo = openRepo() /// sync call


// repo is reactive

janfjohannes avatar Feb 14 '24 17:02 janfjohannes

@janfjohannes This would not be possible because it has an await

This is possible. Every UI framework has a state management solution that handles async init of state.

<script>
  let project = loadProject()
</script>

{#await project}
  <p>loading</p> 
{:then p}
  <p>project loaded {p}</p>
{/await}

Even if not, the following is possible.

let project?: InlangProject


onMount(() => {
  loadProject().then((p) => project = p)  
})


{#if project === undefined}
<p> loading the project </p>
{:else}
<p> project loaded {project.id}</p>
{/if}

Notes

  • initialization of state is async. nothing wrong with that.
  • adapter approach seems doable and better. only one entry point needs to be maintained and communicated. communicating sync and async entry points sounds like a disaster.

@janfjohannes agree that sync for frameworks is not required?

samuelstroschein avatar Feb 14 '24 18:02 samuelstroschein

@samuelstroschein no, but lets discuss this later.

janfjohannes avatar Feb 16 '24 13:02 janfjohannes