opral / monorepo

globalization ecosystem && change control SDK
https://opral.com
Apache License 2.0
1.15k stars 94 forks source link

discussion about reactivity architecture #1122

Closed samuelstroschein closed 1 year ago

samuelstroschein commented 1 year ago

Problem

  1. how to do caching?
  2. reactivity? messages change, how to propagate to linting, updating the UI, etc.
  3. where to "solve the complexity": plugins, apps, or core?

Additional information

NilsJacobsen commented 1 year ago

Take that as a thought experiment. It's not about api definitions, it's about functional responsibilities of the ecosystem. Watch the -> Loom

image

Figjam

samuelstroschein commented 1 year ago

@NilsJacobsen Here is my response loom https://www.loom.com/share/5bcb4bf7e7d14eb5b9bd337ee9a37f4d?sid=99025ec9-8c92-4707-a8ae-e0a57aa26136

CleanShot 2023-07-18 at 16 51 55@2x
NilsJacobsen commented 1 year ago

I always love to hear that the plugins do not get more complexity :D

If the watcher is in core, do we want that core handles file changes. I thought we made a clear cut from files to only messages in core and app? Or only for apps?

Otherwise I really like the watching being part of core. And query being the central interface for apps. Must admit, that I do not know in detail how to establish reactivity for query without using framework specific technology.

Only using reactive CRUD would be absolute best case 👍

felixhaeberle commented 1 year ago

@NilsJacobsen Thank you for your Loom and what you are thinking of the architectural approaches.

I conclude based on our discussion:

Proposal WIP do not comment on it 😂 – it's not finished

// Message Type
type Message = {
  id: string;
  // Define your message properties here
};

// Plugin Type
type Plugin = {
  getMessages: ({ filter }: { filter: Message["id"][] }) => Message[];
  saveMessages: (messages: Message[]) => void;
};

// Messages Type
type Messages = {
  set: (messages: Message[]) => void;
  exec: () => void;
  get: () => Message[];
};

// Core Implementation
const createCore = (plugins: Plugin[], _messages: Messages) => {
  const buildQueryFromPlugins = (): void => {
    const messages: Message[] = plugins.flatMap((plugin) => plugin.getMessages({ filter: _messages.get().map((message) => message.id) }));
    _messages.set(messages);
  };

  const setupWatcher = (): void => {
    // Implementation for setting up a file watcher or any other reactive mechanism
    // Whenever a change occurs, trigger rebuild of the query and persist messages
    // You can use fs.watch or implement your own watcher
    fs.watch('path/to/watched/file', (eventType, filename) => {
      if (eventType === 'change') {
        buildQueryFromPlugins();
        _messages.exec();
        const changedMessages = crud.get();
        persistMessages(changedMessages);
      }
    });
  };

  const persistMessages = (messages: Message[]): void => {
    // Implementation for persisting messages
    // You can use a database, file storage, or any other storage mechanism
    // For example, using SQLite:
    const db = new SQLite('path/to/db');
    db.saveMessages(messages);
  };

  return { buildQueryFromPlugins, setupWatcher };
};

// Plugin Implementation
const createPlugin = (): Plugin => {
  let messages: Message[] = [];

  const getMessages = ({ filter }: { filter: Message["id"][] }): Message[] => {
    // Implementation for retrieving messages
    // Apply any necessary filters based on the provided filter
    return messages.filter((message) => filter.includes(message.id));
  };

  const saveMessages = (newMessages: Message[]): void => {
    // Implementation for saving messages
    messages = newMessages;
  };

  return { getMessages, saveMessages };
};

// App Implementation
const createApp = (messages: Messages) => {
  // access messages
  const messages = messages.get()
};
ivanhofer commented 1 year ago

Here is my first draft how reactivity could look like. It is not finished, just an early draft to start a discussion.

// eslint-disable-next-line no-restricted-imports
import fs from 'node:fs'
import { writable } from 'svelte/store'

function createSignal<T>(arg0: T): [() => T, (a: T) => void] {
    throw new Error('Function not implemented.')
}

function createEffect(arg0: () => void) {
    throw new Error('Function not implemented.')
}

function lintMessages(messages: Message[]): any[] {
    throw new Error('Function not implemented.')
}

function render(lints: any[]) {
    throw new Error('Function not implemented.')
}

function renderUi(arg0: Message[], arg1: Message[]) {
    throw new Error('Function not implemented.')
}

type Message = {
    id: string
    languageTag: string
    pattern: any
}

// plugin api

const initPlugin = () => {
    return {
        // no need to specify "languages" because we can derive it from the messages
        messages: {
            read: (): Message[] => {
                return []
            },
            write: (messages: Message[]): void => {
                // ...
            },
            watch: () => {
                return ['./resources/*.json'] // use some glob library
            }
        }
    }
}

// core api

const initCore = (config: ReturnType<typeof initPlugin>) => {
    // TODO: this would need to be the special object that handles caching, state updates etc.
    const [messages, setMessages] = createSignal<Message[]>(config.messages.read())

    let watcherEnabled = true
    if (config.messages.watch) {
        fs.watch(config.messages.watch() as unknown as string, () => {
            if (watcherEnabled) {
                setMessages(config.messages.read())
            }
        })
    }

    return {
        messages: {
            get: messages,
            set: setMessages,
            onUpdate: (callback: (messages: Message[]) => void) => {
                // ...
            },
            commit: () => {
                watcherEnabled = false
                config.messages.write(messages())
                watcherEnabled = true
            }
        },
    }
}

const setupInlang = () => {
    const config = initPlugin()
    const inlang = initCore(config)

    return inlang
}

// sdk api

const initSdk = (inlang = setupInlang()) => {
    // e.g. convert to svelte store
    const messages = writable(inlang.messages.get())

    createEffect(() => {
        messages.set(inlang.messages.get())
    })
}

// lint api

const initLint = (inlang = setupInlang()) => {
    return lintMessages(inlang.messages.get())
}

const initReactiveLint = (inlang = setupInlang()) => {
    const [lints, setLints] = createSignal<Message[]>(lintMessages(inlang.messages.get()))

    inlang.messages.onUpdate((messages: Message[]) => {
        const changedIds = messages.map(({ id }) => id)
        const newLints = [
            ...lints().filter(({ id }) => !changedIds.includes(id)),
            ...lintMessages(messages)
        ]

        setLints(newLints)
    })

    return lints
}

const initLintCli = (inlang = setupInlang()) => {
    const lints = initReactiveLint(inlang)

    createEffect(() => {
        render(lints())
    })
}

// editor api

const commitButton = null as any
const textArea = null as any

const initEditor = (inlang = setupInlang()) => {
    const lints = initReactiveLint(inlang)

    createEffect(() => {
        renderUi(inlang.messages.get(), lints())
    })

    textArea.onInput((event: any) => {
        inlang.messages.set(event.target.value)
    })

    commitButton.onClick(() => {
        inlang.messages.commit()
    })
}
samuelstroschein commented 1 year ago

@felixhaeberle and @ivanhofer, you are thinking too wide IMO.

@ivanhofer // no need to specify "languages" because we can derive it from the messages

That's already a mistake. Don't derive, don't assume and risk bugs. Let users or plugins explicitly define what languages a project has.

felixhaeberle commented 1 year ago

@samuelstroschein Alright. That's why it matters to talk about implementation. Can you outline what you would suggest as a way to go forward?

samuelstroschein commented 1 year ago

@felixhaeberle Can you outline what you would suggest as a way to go forward?

Yes.

  1. I'll go through batch 1 and batch 2 issues in https://github.com/orgs/inlang/projects/35, excluding the query issue till tmr evening (clean up).
  2. Afterwards, we should have a clearer overview of what we need from query/how we should implement the API.
samuelstroschein commented 1 year ago

Another idea to streamline future adjustments:

const inlang = setupInlang({ config, env })

inlang.config // <- the resolved config
inlang.config.getMessages() // <- manual access to config props if needed
inlang.config.languageTags 

// in the future, we can extend the inlang object as required
inlang.messages.query
inlang.lint.reports
inlang.lint.exceptions 

Pros

Cons

Similar to what @felixhaeberle mentioned, have an inlang object/instance that is returned by setupInlang. Big time question though: How to do reactivity that works in various web apps, VSCode, ... ?

felixhaeberle commented 1 year ago

For me as an developer, accessing everything through one object/instance, makes everything extremely easy. Reactivity needs a second thought here tho.

samuelstroschein commented 1 year ago

New proposal https://www.loom.com/share/13d70a3063ea4959b058fb19c5953556

I also asked on Solid's Discord what they think about it https://discord.com/channels/722131463138705510/751355413701591120/1131182363200725012

ivanhofer commented 1 year ago

I would do config.messages.get() instead of config.getMessages(). In the future we will probably also have config.videos etc.

felixhaeberle commented 1 year ago

Note on the wording: I do not understand why I should access messages through a config. inlang make much more sense here.

samuelstroschein commented 1 year ago

@ivanhofer @felixhaeberle

The inlang instance would be created based on the config.

inlang.config is the "raw" config object. some apps likely need direct access to inlang.config.languageTags etc. inlang.* is the inlang instance.

I would do config.messages.get() instead of config.getMessages(). In the future we will probably also have config.videos etc.

Seems to make sense. But, breaking chang in the config and lint should then also be nested under messages? config.messages.lint?

EDIT: created an issue https://github.com/inlang/inlang/issues/1137

samuelstroschein commented 1 year ago

Very in-depth reply from Ryan Carniato (SolidJS maintainer) https://discord.com/channels/722131463138705510/1131182363200725012/1131266100647174284 on how to tackle reactivity.

felixhaeberle commented 1 year ago

Bringing the discussion points from Discord to github to give a better context.

Talking to Ryan and other members of the solidjs core team, they didn't sell solidjs for our use case that hard. There were a few points to be considered which I listen beneath


Q: Should/can we use SolidJS for reactivity?

Answer


Q: Should we go with a subscribe pattern?

Answer

inlang.messages.query.get({ where: { id: "example-id" }}).subscribe((data) => {
 /* here goes the consumer state logic */
})

Q: Libraries that could help introduce the reactive patterns?

Answer

There are a lot of libraries.

@inlang/team What do you think?

samuelstroschein commented 1 year ago

@felixhaeberle yeah let's go with subscribe pattern but don't expose it to plugins.

Which library

We could go with SolidJS and see if it works given that they plan to provide a dedicated reactive library this year and are the reactivity experts with company backing (Netlify + Google).

Go with SolidJS internally and expose manual .subscribe?

ivanhofer commented 1 year ago

Should we do a little prototype how the implementation would look like using SolidJs?

felixhaeberle commented 1 year ago

@ivanhofer Do you feel confident on providing a first prototype PR ? Then go ahead 👍

I already spoke with @NilsJacobsen about it and we should probably do a starting point (like with the query discussion) and then iterate.

NilsJacobsen commented 1 year ago

I'm also trying to build a prototype.👍 Let's try to finish this topic today.

ivanhofer commented 1 year ago

@samuelstroschein do you have a clue how to make createEffect work outside of the render context? This code will log nothing:

const [value, setValue] = createSignal(1)

createEffect(() => {
    console.log(value());
})
setValue(2)

If we go to the implementation of createEffect, we see that it is empty image

Before we ask stupid questions in the solidjs discord, we would ask if someone knows how to make it work.

ivanhofer commented 1 year ago

If createEffect would work, the API should look somthing like this: https://github.com/inlang/inlang/commit/4e935872b3d68eb271d706354866d4d85f63c122 And here is the same example using svelte's stores, which already provide subscribe/unsubscribe: https://github.com/inlang/inlang/commit/52db1b9a2c8b54847bb5b132505f12db8740fbbf

samuelstroschein commented 1 year ago

@ivanhofer we probably need to define/create a root which usually automatically happens with render. But, can someone handle building the prototype? Then I can focus on other tasks

ivanhofer commented 1 year ago

@samuelstroschein

import { createSignal, createEffect, createRoot } from "solid-js";

const start = async () => {
    const [value, setValue] = createSignal(1);
    console.log(1, value());

    createEffect(() => {
        console.log(5, value());
    });

    setValue(2)
    console.log(2, value());

    await new Promise(resolve => setTimeout(resolve, 100));
}

createRoot(start)

will output

image

samuelstroschein commented 1 year ago

@ivanhofer Seems like we need to wrap the createInlang code in createRoot. Seems like we are good to go then.

ivanhofer commented 1 year ago

@samuelstroschein no, createRoot has no effect in the example. Effects never run.

samuelstroschein commented 1 year ago

@ivanhofer i was about to edit by comment :D indeed odd. works on the solid playground. I'll invest 10 mins to see if i get it to run

samuelstroschein commented 1 year ago

@ivanhofer can't get it runing. can you use this stackblitz https://stackblitz.com/edit/stackblitz-starters-gxm7op?file=index.js and ask on the solid discord?

ivanhofer commented 1 year ago

Ok, I will do that.

Meanwhile I did some experiments with S.js and it looks promising.

samuelstroschein commented 1 year ago

@ivanhofer why S.js and not something (more) maintained like Nanostores?

ivanhofer commented 1 year ago

Becasue of the auto-tracking of dependencies. No need to define a dependency array. This is also one of the main benefits why solidjs would be appealing.

ivanhofer commented 1 year ago

While we are waiting for an answer from solid, I want to share some thoughts:

The auto-tracking would be great, but if we choose to provide a .subscribe function, we should probably go with a solution that supports it natively. Comparing solid.js and Svelte, having a library with native .subscribe requires less manual code. The only question is how do we handle unsubscibes, to not introduce memory leaks?

felixhaeberle commented 1 year ago

Interesting question. Because we do not have a component context here, we can't make use of effect callback on dismount.

We could either:

I'm more on the side of 1

Talking about memory leaks: I would simply ignore this topic for now and assume that people know how to write their apps properly. If we find out they can't, I would care about it in a later stage. If we introduce the unsubscribe pattern and advise apps to do it, memory leaks also become less likely.

NilsJacobsen commented 1 year ago

Did discuss that with @ivanhofer.

Could we define the API at this point so that other components of the core, apps, and plugins can begin working with it?

Example:

export type MessageQueryApi = {
    create: (args: { data: Message }) => void
    get: (args: { where: { id: Message["id"] } }) => { 
        message: Message | undefined, 
        subscribe: (callback: (message: Message | undefined) => void) => void 
    }
    update: (args: { where: { id: Message["id"] }; data: Partial<Message> }) => void
    upsert: (args: { where: { id: Message["id"] }; data: Message }) => void
    delete: (args: { where: { id: Message["id"] } }) => void
}

We could agree on the subscription approach, maybe also think about unsubscribe, but then merge it and let one person work on reactivity while the rest is finishing core/ apps/ plugins.

That way we do not have to wait for answers in discord ...

samuelstroschein commented 1 year ago

Steve (Wozniak) comes to the rescue.

Here is working reactivity in 20 lines of code with subsribe auto disposal etc. :) https://stackblitz.com/edit/stackblitz-starters-okatwd?file=index.js . @ivanhofer a signal with a subscription is 7 lines of code. @inlang/editor the get() should automatically be reactive without subscribe 😎 .

Leeeeeet's go. Please vote on SolidJS with the subscribe wrapped signal. Still needs some hacking with types but that should be doable!


@felixhaeberle Talking about memory leaks: I would simply ignore this topic for now and assume that people know how to write their apps properly.

Nope. Choose a reactive library that eliminates the potential for memory leaks.

felixhaeberle commented 1 year ago

@samuelstroschein Steve is on fire – nice API!! 🔥

offtopic: why import from dist 🧐 do they not have proper exports?

samuelstroschein commented 1 year ago

@felixhaeberle

The solid js package.json imports dist/server.js by default which is overwritten to dist/solid.js in the browser. Since we are running the code above in a server context, the code the server code was imported.

https://discord.com/channels/722131463138705510/1131182363200725012/1133666569558425700

ivanhofer commented 1 year ago
const message = query.get()
// or 
query.get().subscribe()

I don't think this is possible.. the Message and .subscribe can't exist on the same object, or else it would break serialization, which we probably need?

This would be the solid syntax:

query.get({ })() // get value
query.get({ }).subscribe()
samuelstroschein commented 1 year ago

@ivanhofer yep already removed!

Maybe we should just export createEffect for apps. We want reactivity not only for messages but also config props for example.


createEffect(() => {
  const config = inlang.config.get()
})

Or getters take a on prop (because only applies to getters)

const config = inlang.config.get()

// need to double check if this mutation is possible. 
let config = inlang.config.get({  onChange: (_config) => {config = _config} })
felixhaeberle commented 1 year ago

@samuelstroschein Do you speak about https://www.solidjs.com/tutorial/reactivity_on ?

ivanhofer commented 1 year ago

Should we just go without .subscribe and see if we run into problems? We could provide a helper function to convert a signal to a subscription, if someone really needs it. If we have problems, we can always add a subscribe function. Having effects and subscriptions at the same time will probably cause some issues

ivanhofer commented 1 year ago

I think this is the API we should go for: https://github.com/inlang/inlang/blob/reactivity-solid/source-code/core2/src/test.ts

It uses Signals, Memos and Effects, is performant and easy to read. Create root can be called when initializing an application.

I tried it also with stores, but the store API is weird and I also could not make it work ^^. I find the signals api way easier to reason about. We can meet in around to discuss my findings if you want. Just ping me on discord.

ivanhofer commented 1 year ago

Did some testing and it has absolutely no effect whether createMemo get's called multiple times within an createEffect. So it looks like we can safely hide the accessor of signals and return the raw value and stil lbe able to offer reactivity on top 🤯: I'm impressed by solids implementation 👏

samuelstroschein commented 1 year ago

Closing as "we are going to use SolidJS".