search

opral / monorepo

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

collection requirements Reactivity/State management in the SDK #1772

Closed samuelstroschein closed 7 months ago

samuelstroschein commented 11 months ago

This issue is updated as new requirements unfold.

Problem

We need to find a better-suited state (reactive) management solution for the SDK. That solution could be a "better way" of dealing with signals, another library, or an entirely different approach. To be determined.

Context

The SDK currently uses (Solid) Signals for state management/reactivity. We (knew we would) face numerous issues that pimple up now to the extent that we are inclined to take action.

Requirements

KraXen72 commented 11 months ago

using solid signals for an sdk is kind of crazy lol. surprised it managed to live for this long 😅 good luck towards finding a better, more stable solution

NiklasBuchfink commented 11 months ago

I think this is more of a cleaning task and making sure everything is set and passed as it should. We didn't even use stores, because it is complex to understand, but this could help eliminate this issue with handling complex data like our linting.

There is room for improvement. I dug more into it in the last days with podcasts and having the Solid docs as good night reading :)

samuelstroschein commented 11 months ago

using solid signals for an sdk is kind of crazy lol. surprised it managed to live for this long 😅

The SDK has the odd requirement that close to every call is reactive.

Getting the name of a project, querying a message, getting project settings, installing a plugin is/are all reactive properties that should bind to a GUI without leading to constant re-renders. Otherwise, the fink editor and parrot will be noticeably slow.

The perfect match for our use case does not seem to exist. The question is: What solution comes somewhat close?

samuelstroschein commented 10 months ago

Some research over the holidays:

samuelstroschein commented 10 months ago

i just got off a call with Datner regarding effect ts , see his comment here

mobx vs solid signals

Both are similar by creating observable state i.e. createSignal() or makeObservable(). Pros I see over solid signals:

Cons I see:

longer discussion today on discord too with input from @janfjohannes https://discord.com/channels/897438559458430986/1192097871613399110/1192149790256017438

samuelstroschein commented 10 months ago

New hard requirement: The solution should not "leak reactivity" to consumers.

This requirement has been forgotten but was a large driving force for Signals. The SDK should work in plain JS without quirks: 

// this should work
console.log(project.errors)

// requiring a reactivity pattern should not be required!
project.errors.subscribe((e) => console.log(e))

This is a strong reason to choose MobX, which relies on proxies to achieve "reactive magic" but looks like and can work like vanilla JS. The benefits of MobX over Signals have been explored here https://github.com/opral/monorepo/issues/1772#issuecomment-1875981167.

This requirement got "re-emerged" in https://github.com/opral/monorepo/issues/1680#issuecomment-1877742428

samuelstroschein commented 10 months ago

I stumbled on https://github.com/artalar/reatom which might also be a worth a look

cc @janfjohannes because we chatted about effector

samuelstroschein commented 10 months ago

Discussions with @martin-lysk that implicit reactivity (MobX & Signals) seem undesirable

https://www.loom.com/share/260363100ff34d3f850f8a4913c851b5?highlightComment=28441771&t=93

CleanShot 2024-01-09 at 23 33 51@2x
samuelstroschein commented 9 months ago

@jldec wrote in a check in https://discord.com/channels/897438559458430986/1084748995349446667/1202188931362193417

initial thoughts on reactivity - the apis in solid are not helping us, and I'm not convinced that other similar abstractions like reatom will help us either. My instinct (not fully confirmed) is to try to eliminate the extra abstraction layer and use explicit callbacks where possible. Automatic dependency tracking based on events is not buying us enough to justify the hidden additional complexity.

We started the SDK without reactivity but needed it to build UI layers with the SDK. It seems like you (like @martin-lysk and @janfjohannes before you) overlooked this requirement?

There needs to be a reactivity pattern that propagates changes to consumers in a correct manner. The SDK state is not isolated to itself. I emphasize correct computations here. The majority (all?) of the SDK state is async. Parallel async computations that depend on each other are the norm. Trying to coordinate them manually sounds like we will inevitably build our own state management solution with many bugs along the road.

I agree that Solid/Signals is not the right solution. We took them as a quick and dirty way to achieve bindings from the SDK state to the @inlang/fink-editor UI layer. Another requirement btw. Whatever solution we choose needs bindings to UI frameworks like reatom, mobx & co already have https://www.reatom.dev/package/npm-react/. Developing and maintaining such bindings manually = we are building our own state management solution, which seems out of scope.

samuelstroschein commented 9 months ago

Here is an example of "reactivity seems required".

Also re-stating that this discussion and the comment regarding "reactive system is somehow needed to propagate changes to consumers" (https://github.com/opral/monorepo/issues/1772#issuecomment-1919193325) applies 1:1 to @opral/lix.

With reactivity in lix

// the block needs to be re-executed if any of the following happens:
// [currentBranch changes, settings file changes, uncommitted changes changes]
//
// This is a trivial five lines of code example, but it goes to show how complex scheduling
// computations in the SDK and lix is  

// ----- BEGIN OF BLOCK ------

// the conditional must be re-run the moment the current branch changes
if (repo.currentBranch !== "main") {
  // loadSettings needs to re-execute if the settings file changes
  await loadSettings()
  // console.log needs to-rerun if uncomittedChanges "changes" 
  console.log(repo.uncommittedChanges)`
}

// ----- END OF BLOCK -----

async function loadSettings() {
  const file = await repo.fs.readFile("project.inlang/settings.json")
  // ...
}

Without reactivity in lix


// (endlessly) deep nesting required 

repo.currentBranch.subscribe((branch) => {
  if (repo.currentBranch !== "main"){
     repo.fs.watch("project.inlang/settings.json", () => {
       await loadSettings()
       repo.uncommittedChanges.subscribe((changes) => {
         console.log(changes)
       }
      })
  }
})

async function loadSettings() {
  const file = await repo.fs.readFile("project.inlang/settings.json")
  // ...
}
KraXen72 commented 9 months ago

for unsubscribing, you could use svelte stores' approach, where a subscribe call returns an unsubscribe function. (i don't know the details off the top of my head, better to check their docs)

jldec commented 9 months ago

@samuelstroschein thanks for the additional details regarding lix. Could you also clarify (or point to issues) why the current lix fs watch api does not meet our needs?

I take it from this comment that a watcher was intended here.

for reference - the fairly mature chokidar api.

samuelstroschein commented 9 months ago

@jldec Could you also clarify (or point to issues) why the current lix fs watch api does not meet our needs?

It does.

My intent was not to surface a problem in the fs API but to show that manual subscriptions will lead to callback hell and bugs. I updated my comment and changed repo.fs.readFile.subscribe to repo.fs.watch.

Did I answer your question?

(Not to be discussed in this issue)

We could make the point that repo.fs.readFile() should be reactive for the same reasons we want project.* and repo.* props to be reactive: Avoid callback hell, manual diffing, and reduce the likelihood of bugs. Manually setting up watchers and diffing already led to several bugs. A reactive repo.fs.readFile() seems like it avoids code like https://github.com/opral/monorepo/pull/2108/files#r1473518448

jldec commented 9 months ago

From https://github.com/opral/monorepo/issues/1772#issuecomment-1919193325

It seems like you ... overlooked this requirement?

@samuelstroschein regarding the SDK api for syncing with UI, it's clear that we need something there - definitely not overlooked, in fact I expect we'll keep using solid in our UI tier - just not all the way down to the sdk internals.

No firm opinion yet, but I would probably start a POC using somthing similar to one of these database pub-sub apis. .e.g

samuelstroschein commented 9 months ago

in fact I expect we'll keep using solid in our UI tier

New projects use https://lit.dev/. We want to phase out solid. Just use web components and be done. Any custom renderer like Svelte, React, Solid, Vue is too expensive nowadays. Expensive referring to compatibility issues across large projects.

POC using somthing similar to one of these database pub-sub apis. .e.g

But why?

  1. There is no need for a server. The beauty of inlang is precisely having no server. The state is small enough to be handled client-side which removes the entire server layer and makes automation, extension, and collaboration easier. Read this.
  2. If you mean you want something similar to pub-sub [but client-side], then this is reactivity. Building our own solution for reactivity seems redundant. RxJS, MobX, Effector, Reatom, solve the problem of "here is a change" -> "you can listen to that change and trigger side effects".

Let me know if we should hop on a call. There seems to be un-alignment regarding the architecture of the SDK.

jldec commented 9 months ago

Alignment :)

NiklasBuchfink commented 9 months ago

I heard in a podcast today that the TC39 works on signal standardization. I hope this is the proposal they were referring to, but it's a good idea to keep it in mind. If a standardization for reactivity and signals comes up, we'll probably adopt it:

samuelstroschein commented 9 months ago

Here is an example test that should (must?) pass https://github.com/samuelstroschein/poc-sdk-reactivity with pseudocode, test cases, and a real implementation.

cc @janfjohannes @jldec

martin-lysk commented 9 months ago

Thanks for the poc and the loom. I thought about this and i think we are on a wrong track here.

While we all agree that we need to find a better simpler way to express the dependencies than we have in the sdk I think we mix up initialisation with a running SDK Project.

Background: We try to pack into the sdk is asynchron state. But while async and await have simplified triages of api fetches and error handling a lot easier, the underlying concept can become hard to understand / debug and. grasp.

Example 1: You can create a deadlock in a single thread env. ??? Yes two promises can await each other end never resolve. While the main loop is not blocked your pretty async function may never return.

Example 2: The synchronous part of a async function gets called immediately when you call the function but the result is processed only after the next tick if you await it. 

async function foo(name) {
  console.log(name, "start");
  await console.log(name, "middle");
  console.log(name, "end");
}

foo("First");
foo("Second");

Whats printed out? What do you think first - what is your second guess? Can you explain it without looking at spec again.

console First start First middle Second start Second middle First end Second end

Important things to notice: an async function is not asynchronous if it has no await statement inside of it, the order of the execution can not be known from outside without the knowledge of the implementation details. Even if you control the flow from outside - microqueues can always hop in and say - NO NO NO.

I guess this reflects my first intuition here: https://github.com/opral/monorepo/issues/1846#issuecomment-1848609181 and i try to be more precise on this.

I would say CRUD operations in the SDK should be sync. yes this is just half of the truth. No matter how hard we try - the persistence will always be async*. But these are two things. Changing properties in memory - sync and persisting those changes to the disk - async.

We load the data into the project object - we keep settings, messages etc. in memory - and whenever we face a change on those objects/properties we persist the changes to the corresponding files on disc again. When we detect a change (fs.watch) we load the changes from the file again. We synchronize but we don't have a guarantee that memory representation and disk are in sync.

Take the case you outlined in the loom @samuelstroschein, settings change - a language tag was changed. Nothing prevents a separate process to write to the settings file while we do a computation on the messages. Not even checking the settings file before each operation would help us here - we would again end up with a lock file to synchronize our "async" operation. While those simple tests in the POC run green in your env - on a real env with multiple processes messing around with your settings file - this will become flaky again.

Proposal

  1. A DAG that describes the dependencies of properties in the project.
  2. An async load function and reactive subscribable properties - similar like the once we are having at the moment.
  3. continue to have synchronous CRUD operations that reflect the changes directly after the execution but return a promise that stores the changes to disk
  4. We use the dependency DAG to describe and detect conflicting changes on the disk that clash with changes in memory.

Take the current implementation of setSettings in loadProject. This is currently synchronus but it doesn't handle errors during disk writes. Also the write to the disc is not finished before the function returns.


-- const setSettings = (settings: ProjectSettings): Result<void, ProjectSettingsInvalidError> => {
++ const setSettings = (settings: ProjectSettings): Promise<void> => {
--          try {
                const validatedSettings = parseSettings(settings)
                _setSettings(validatedSettings)

--              writeSettingsToDisk(validatedSettings)
--              return { data: undefined }
++              return writeSettingsToDisk(validatedSettings)

--          } catch (error: unknown) {
--              ..
--          }
        }

Whats cool about this approac. The function is not marked as async - also the affect takes place directly.

If you set the Language tag by calling setSettings - the new language tag will become visible directly, all subsequent call will work on the new language tag AND the persistence of the new settings is already scheduled and we return the promise that will take care of this. There is no need to synchronize - since its synchron. You can't see a different value as long as you are on the same stack.

This comes with some nice features:

1.) If save messages to disc is throttled you could batch all changes together using Promise.all 

await Promise.all([
messageQuery.update({where: { id: "1" }, data: ... };
messageQuery.update({where: { id: "1" }, data: ... };
messageQuery.update({where: { id: "1" }, data: ... };
])

2.) If a change should to hit the disc before we can move on - one can use await that ensures that the returned promise - which stores the changes to disk gets resolved before we move on. await project.setSettings(...)

3.) An exception that happens on the message obejct in memory will be thrown into the callee function and doesn't need to be catched using .catch(). ...

janfjohannes commented 9 months ago

@NiklasBuchfink btw pretty sure the decorators you linked to is not what they meant with signal standardization, would be great to find the spec in question.

martin-lysk commented 9 months ago

Linting, full text search and other computation heavy operation got to be async for sure. But If it comes to the raw message handling - like CRUD there is one big plus on sync: atomicity.

If we make CRUD async a simple two operation like this can fail:

const messageToUpdate = await project.query.messages.get({ id: "b"});
const messageToUpdate.variants[0].pattern[0].value = 'updated text';

// upsert is async -> we schedule it to the next tick 
// -> an update or a deletion comming from an external event 
//    like fs.watch will not know about the planned update and change message with id = b in between
await project.query.messages.update({
  where: { id: "b"},
  data: messageToUpdate
}) // <-- this can throw and you don't know why

If we have CRUD sync and only the persistence done async we can handle those cases in persistence layer where all information is available.

samuelstroschein commented 9 months ago

@martin-lysk

I thought about this and i think we are on a wrong track here.

All you propose is what I have in mind. Not sure where we are disagreeing. This discussion is more about the reactive state management solution that we should adopt to help us with:

Reatom's scheduling feature seems to be more or less what you describe with automatic dependency tracking, atomicity, and cancellation.

Furthermore, I think we can rule out MobX and Effector (not Effect TS) based on the insight that the main problem seems to revolve around async side effects that need be "bottoms up cancellable". Both MobX and Effector have no indication of solving that problem. Here is an example:

// PSEUDOCODE
async function updateMessage(args) {

  // three side effects. if one of them fails, the entire computation must fail.
  //
  // the ctx.schedule pseudocode API takes care of handling this.
  return await ctx.schedule(async () => {

    // ⚠️ succeeds but needs to be rolled back
    await updateStorage(args) 

    // ⚠️ succeeds but needs to be rolled back
    updateMessagesMap()

    //💥 throws. the schedule needs to be canceled "bottoms up" 
    //    `updateStorage` and `updateMessages` must be canceled too 
    //     to not leave the user with a broken project!
    await executePluginsExporters(args) 
  })

}

your proposal makes CRUD operations async [by returning a promise], which aligns with #1846 :D

CleanShot 2024-02-11 at 20 20 23@2x

  1. you proposal returns a promise?
  2. yes, that's why #1846 exists. side-effects need to be awaited.

1846 argues that any CRUD (really any property of project.* btw) API must return a promise because apps need to await all async side-effects like persistence to storage. Otherwise, apps are buggy. A user changes a message, the app does not await async side-effects like persistency and wups the user, and inlang app developer wonders why the message is never persistent.

The following code is a naive bugfix for https://github.com/opral/monorepo/issues/1968 because CRUD ops do not return an promise: "Please let 4 seconds be enough to await all side effects":

https://github.com/opral/monorepo/blob/34b29ddc73993b41da8541367a5beb21c0f92482/inlang/source-code/cli/src/commands/machine/translate.ts#L133-L135

In case you really argued to leave CRUD functions sync, the bug above gives me 100% confidence that CRUD ops need to be async and await side-effects to avoid bugs. Issue #1846 already identified multiple bugs.

samuelstroschein commented 9 months ago

We should probably open a google doc/spreadsheet to collect requirements at this point and have a matrix comparison of potential solutions.

martin-lysk commented 9 months ago

I guess i understand the confusion here:

I am not proposing not to return a promise I am proposing make the crud functions non async functions that return a promise.

SDK functions would have two phases:

1) changing state I suggest that changes in the SDK should be synchronous. Namely, a call like setLanguageTag('en') should instantly change the language tag synchronously, and the effect should always be reflected in the state of the SDK in the current call stack and in the current tick. It should even throw synchronously if you use the function incorrectly (e.g., setLanguageTag('bla')).

2) persisting state After the state has been updated it should return a promise that takes care of the persistence

Please ignore that i use class here and we will most likely replace the persistence method with an implementation backed by reatom or another framework and it simplifies a lot like error handling but i hope it help to grasp the idea behind it.

class Project {

   static async loadProject(inlangFolder: string, lix: Lix) {
      // load settings ...
      const settings = await loadSettings();
      // load messages ...
      const messages = await loadMessages(settings);
      // setup (synchronous) reactivity on properties using a DAG
      // ... the frame work that we choose at the end

      // connect external effects
      lix.watch(inlangFolder, () => {
           // trigger effect based on the changes
      })

      return new Project(settings, messages)
   }

inlangFolder: string,
  lix: Lix,
  settings: Settings,

  messages: Messages,

   updateRefLanguageTag(languageTag: string) {
      setting.refLanguage = languageTag;

       return (async () => {
             await lix.writeFile(inlangFolder + '/settings.json');
       })()
   }

   getMessage(id: string) {
     return message[id]
   }

   updateMessage(id: string, data: any) {
       if (id.split('_').length != 4) {
         // this is a sync exception that can get caught in a sync env as well!
         throw ne Error("Id must contain 3 underscores"); 
       }
       messages[id] = data;

      // we can and should use reatom or another library to schedule the save and allow it to be canceled
      // this should only point to the fact that we don't use an async function here and the that changes to messages hit the SDK before we write the change
       return persistMessages()

   }

   // reactive property that the ui can subscribe to
   persistenceError: Error | undefined

   scheduledMessagePersistence: Promise<void> | undefined

   persistMessages() { 
        if (this.scheduledMessagePersistence) {
          return this.scheduledMessagePersistence
        }
        this.scheduledMessagePersistence = (async () => {
                 this.scheduledMessagePersistence = await undefined;
                // ... do the persistence 
        });
        return this.scheduledMessagePersistence.catch(e => persistenceError = e;) 
   }
}

Usage in an async context - where we don't want to have a persistence roundtrip for every update

const inlangProject = await Project.loadProject('project.inlang', lix);

try {
   const update1 = inlangProject.updateMessage('test_test_test_test', {...})
   // changes from update 1 are already visible to update 2, even exceptions because of a miss use like wrong id would be visible to update two already
   const update2 = inlangProject.updateMessage('test_test_test_test', {...}) 

   // persistence has not yet started and we can await it  
   await Promise.all([update1, update2]);
} catch (e) {
   // error on usage of the api
}

Usage in an async context - where we want the change to be persisted to disc BEFORE we continue

const inlangProject = await Project.loadProject('project.inlang', lix);

try {

   await inlangProject.updateMessage('test_test_test_test', {...})

   await inlangProject.updateMessage('test_test_test_test', {...}) 

} catch (e) {
   // error on usage of the api
}

Usage in a sync context like a callback

const onVariantChange(messageId, languageTag, selector, pattern) => {
    const message = project.getMessage(messageId).getVariant(languageTag, selector);
    message.pattern = pattern;

    // persistence errors will be handled catched by the project and propagated via the reactive error property
    const updateMessagePersistence = project.updateMessage(messageId, message);

   // if we want to directly give feedback here -> we can use the returned promise:
  updateMessagePersistence.then(() => alert('yeah everything saved')).catch((e) => alert('something went wrong ' + e.message); 
}
NiklasBuchfink commented 9 months ago

@NiklasBuchfink btw pretty sure the decorators you linked to is not what they meant with signal standardization, would be great to find the spec in question.

@janfjohannes You are right, I asked for resources, this was the answer: https://twitter.com/AtilaFassina/status/1757048374140796952

The video gives a loose indication of where the signals are heading. There has been a proposal draft for observables in the past, but for signals nothing concrete yet. It's still WIP.

samuelstroschein commented 9 months ago

Sentiment from my after the call earlier today:

Reatom seems to address @martin-lysk's raised issues of concurrency and batching natively.

Take a look at the concurrent API and async package. Both APIs seem to specifically address Martin's concerns about handling concurrent updates. In contrast to Martin's sync proposal, reatom's API comes with explicit handling of cases such as "last-in-win", or rollback. Reatom's APIs are a bit convoluted/the docs not crisp, but it seems like it solves our issues better than other solutions:

Notes

cc @martin-lysk @janfjohannes @jldec

samuelstroschein commented 8 months ago

@opral/lix this requirement and example for reactive state came up in the github action that @NiklasBuchfink is building today:

The project should not be required to be re-opened because the UX in an app like @opral/inlang-fink can become miserable if the project load takes 20 seconds while a reactive branch switch could only take 3 seconds (fictive numbers) 

const repo = await openRepository({ branch: "main" })
const project await loadProject(repo)

// expect: lint reports on main branch
const reports1 = project.query.messageLintReports.getAll()

await repo.changeBranch("fluffy-gummybear")

// expect: lint reports on fluffy gummybear branch
const reports2 = project.query.messageLintReports.getAll()
janfjohannes commented 8 months ago

@samuelstroschein of course! this behaviour is allready part of my POC and totally on the same page. fixing the current requirement to reopen is tracked in this bug : https://github.com/orgs/opral/projects/45/views/1?pane=issue&itemId=53434606&sortedBy%5Bdirection%5D=desc&sortedBy%5BcolumnId%5D=Status

artalar commented 8 months ago

Hi! Reatom author there. I would like to help you with this discussion, if you don't mind :)

Concurrency, rollbacks, async management

Based on what I understand from this discussion, here is a list of points you might find interesting:

About ecosystem preservation and stability

To be clear, I am really tired of the fragmentation and stability of the React ecosystem and am trying to build something opposite with Reatom. We want to cover all common frontend tasks with a set of well-coupled primitives. I expect that a normal Reatom user will not have any headaches about which library to choose for one or another task, because we will have almost all needed features. And the important thing is that we try to build all packages with enterprise-grade quality. In the last half year, for one site builder I worked on, I personally developed a new forms package, a router package, a zod package (reactive model generations from a zod contract with automatic changes synchronization), and it will be released soon. But there are a lot of contributions with new features from other contributors too! We are very welcome to increase our ecosystem and we want to store it all in a stable monorepo. And you should know that you can always write your own alternative solution as the primitives (atoms, ctx, hooks) are public and it is powerful enough.

Currently we have a paid maintainer for some kind of tasks, but we plan to move to https://polar.sh to open doors for other involved people.

In the next few years we plan to grow and grow. Reatom is the core tool of many long-lived projects that it's users and I care about. But it will always be a project with a heart, the heart that cares about web stability, performance and quality.

About the future

In this year, we will move from explicit ctx to ponyfiling exact AsyncContext, leaving all current features, and bring new incredible ones. One of the most important new features is algebraic effects, like React Suspense. It will be an optional feature, of course. These changes will simplify code style and types significantly, while still remaining robust due to runtime checks, cause tracking, and additional ESLint checks.

If you have any questions, please, ping me :)

samuelstroschein commented 7 months ago

@artalar Thank you for the reply!

We figured that we need multi-threading (via workers) to fulfill our state/computation management requirements. Instead of hyperoptimizing the main thread, we will likely go for:

  1. Workers compute stuff
  2. Bridge between workers and main thread
  3. Main thread exposes some reactivity bindings

I am unsure how reatom would help us with 1 and 2. Maybe for 3 it is a good choice. That will be decided by the person evaluating an architecture for the state management solution we come up with.

artalar commented 7 months ago

@samuelstroschein, for example, you can use withBroadcastChannel to synchronize data from a worker to consumers. For posting data processing, you can use actions that will synchronize via postMessage (we could develop an adapter for that). The main idea and benefit is to write the logic once but have the ability to run it in different contexts (worker/tab).