Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getter functions will lead to multiple breaking changes in the future #1680

Closed
samuelstroschein opened this issue Nov 21, 2023 · 9 comments
Closed
Assignees
Labels
scope: inlang/sdk Related to source-code/sdk. scope: lix Related to source-code/git-sdk.

Comments

@samuelstroschein
Copy link
Member

Problem

The Subscribable approach of the project and @inlang/lix API has a high risk of breaking changes.

We settled on getter functions because most signal implementations use getter functions. However, determining whether a property is or will be reactive in the future yields a high risk of making a wrong choice.

// reactive getter function
project.errors()

Example

#1678 will add a project.name property. Shall project.name be reactive?

Should users have the possibility to rename a project in a UI like the Fink editor without reloading the entire project? Arguably yes. Is that use case obvious at the moment? No. So what to do now? Make the property reactive or not?

If we decide to make project.name static and reactive in the future, we have a breaking change. If we make project.name reactive now but it will remain static in the future, developers expect project.name to be reactive while it is not.

@maige-app maige-app bot added effort: medium scope: lix Related to source-code/git-sdk. type: improvement Something that can be improved. labels Nov 21, 2023
@samuelstroschein samuelstroschein added scope: inlang/sdk Related to source-code/sdk. and removed type: improvement Something that can be improved. effort: medium labels Nov 21, 2023
@samuelstroschein

This comment was marked as outdated.

@samuelstroschein
Copy link
Member Author

@martin-lysk @janfjohannes curious about your input regarding the proposal above. another proposal is also welcome.

a bummer that JS reactivity is not standardized. @martin-lysk please do not open a discussion if reactivity is required or not :D we made the experience that building the @inlang/editor became so much easier with reactivity that it is/was well worth it.

@NiklasBuchfink @NilsJacobsen if you disagree with the reactivity made building the Fink editor easier, please let us know

@samuelstroschein samuelstroschein self-assigned this Nov 21, 2023
@NilsJacobsen
Copy link
Member

For fink it was great 👍

@NiklasBuchfink
Copy link
Member

Lol, I heard this exact discussion this morning :D
https://open.spotify.com/episode/0WOjZzBZ3yDsDIuEvcbCAy?si=50e4c6641399441a
Timestamp: 24:06

Agree, for SolidJS in fink it was great 👍

@janfjohannes
Copy link
Contributor

@samuelstroschein i dont understand the problem description fully. but i still think we should not poolute lix with any framework specific reactivity concepts, the implementation we settled on was a compromise and not pretty but mostly motivated by having to change the editor as little as posible and i see the error object still as experimental until we know more about how to best use lix and reactivity.

i would say that the standard js ways to get a stream of values would be (from older to more modern, leaving out observables):

repo.on('error', callback)
repo.errors.subscribe(error => {....})
or for await (const error of repo.errors) {...} < my current favorite as this also is in the js standard and supported by all browsers

to adapt this to whatever framework reactivity system needs should be not part of lix or maybe some extra exports in a seperate module.

@samuelstroschein
Copy link
Member Author

I will update the issue on the problem later today.

The issue is unrelated to the implementation. The API we expose of Subscribable is of high risk for breaking changes.

@samuelstroschein
Copy link
Member Author

I will update the issue after I implemented the directory proposal.

@samuelstroschein
Copy link
Member Author

samuelstroschein commented Jan 4, 2024

reply to @janfjohannes #1680 (comment)

but i still think we should not poolute lix with any framework specific reactivity concepts [...]
to adapt this to whatever framework reactivity system needs should be not part of lix or maybe some extra exports in a seperate module.

No worries, we are not going to leak framework-specific reactivity.

The fact is that we need a reactivity/observable solution in the SDK. The inlang SDK and lix need to "broadcast" events to consumers [which triggers side effects like rendering the UI]. How those side effects are triggered can be a React wrapper, Solid wrapper, whatever. In any case, the SDK needs a primitive to expose those events.

for await (const error of repo.errors) {...} < my current favorite as this also is in the js standard and supported by all browsers

Async iterators seem unsuited. They are a pull-based mechanism. A UI doesn't know when to pull changes. Updating UIs requires a push-based mechanism. Observables and signals are push-based. We need "(SDK/lix to UI) Hey UI, project.errors changed" and not "(UI to SDK/lix) Hey SDK, did project.errors change?". The latter would require manual pulling all over the source code, leading to engineering complexity and a degraded user experience. The UI would not be guaranteed to react to changes in a project [because the manual pulling implementation has a naive 1s timer for example]

PS Lix has the same requirements. If an app calls repo.statusMatrix() to display changes in the UI, the call must be reactive/observable.

EDIT: signals are, interestingly enough, push and pull-based at the same time. Pull-based because no subscription is required e.g. an app can just access project.errors() with a push mechanism in the form of effect, which triggers reactive updates

EDIT 2: Edit 1 lead to #1772 (comment). Thanks @janfjohannes for raising your concerns about not leaking implementation details. The comment prompted me to formulate the requirement in the SDK

@samuelstroschein
Copy link
Member Author

samuelstroschein commented Feb 9, 2024

We partially made the right design decision by settling on functions.

Good:

  • static access like project.errors does not work. at any given time an async computation could occur from which errors are derived from. the parent computations must be awaited before project.errors returns a value. Hence, project.errors() must be an async function!

Bad:

  • every property must be an async getter because every property depends on an async parent computation like reading the settings file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: inlang/sdk Related to source-code/sdk. scope: lix Related to source-code/git-sdk.
Projects
None yet
Development

No branches or pull requests

5 participants