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

Non reactive errors #2548

Merged
merged 15 commits into from Apr 16, 2024
Merged

Non reactive errors #2548

merged 15 commits into from Apr 16, 2024

Conversation

martin-lysk
Copy link
Contributor

@martin-lysk martin-lysk commented Apr 9, 2024

Lix currently returns a reactive property called errors. With this one Lix has 3 kinds of error handlings:

  1. Function throws
  2. Function catches and adds errors to the errors property
  3. Function catches and returns an errors object as function result

This PR removes the second case for now and moves error collection logic to the thin layer using the lix SDK.

In the future we will have defined errors returned in the return object of a function called. (compare 3.) for errors that the client can react on and can deal with and throw for unexpected errors like asertation errors wrong url formats or state issues (file system has changed unexpectedly).

Copy link

changeset-bot bot commented Apr 9, 2024

⚠️ No Changeset found

Latest commit: ebb3b76

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@janfjohannes

This comment was marked as resolved.

@martin-lysk

This comment was marked as resolved.

@martin-lysk martin-lysk marked this pull request as draft April 10, 2024 11:33
@janfjohannes

This comment was marked as resolved.

@samuelstroschein
Copy link
Member

samuelstroschein commented Apr 10, 2024

@martin-lysk In the future we will have defined errors returned in the return object of a function called.

Please elaborate.

If you refer to exposing a "result" like type e.g. const { data, error } = await repo.foo(), I urge reconsideration. The JS ecosystem expects throws. Error handling in frontend frameworks for example, will not trigger anymore. While you could argue that "but frontend frameworks will get a wrapper", the problem is caused by steering away from standards. I know that error handling in JS is frustrating but we win nothing by deviating from the standard.

Supabase switching to result types supabase/supabase-js#32 got overwhelmingly downvotes after the change was merged. Counter proposals are happening supabase/supabase-js#885 to revert the change are happening too.

In short, while Result is nice internally, exposing a result type externally is breaking standards and thereby compatibility.


EDIT: Following my "breaking compatibility" argument, maybe we should get rid of repo.errors and project.errors to align with expectations and increase compatibility.

  • every function throws including loadProject() or openRepo() if an error can't be handled
  • if needed, introduce repo.debug or project.debug for "errors" that are none fatal e.g. "falling back to local repo, etc."

cc @opral/inlang-sdk leaking result types to the consumer applies to inlang's message SDK too


EDIT 2: repo.errors and project.errors was introduced because CRUD ops were sync while needing to wait for side effects. Making CRUD ops async to await side effects, removes the need for project.errors altogether.

Proposal

Remove repo.errors in favor of making functions throw and a repo.debug log for noncrucial errors/warnings.

Making all functions that trigger side-effects async is required to make functions throw. I argue it's the right approach because a) functions with side effects are async by definition and b) error handling becomes easier.

# Conflicts:
#	pnpm-lock.yaml
@martin-lysk
Copy link
Contributor Author

martin-lysk commented Apr 10, 2024

@samuelstroschein Jan and I concluded this a the way to go yesterday. @janfjohannes please hop in with your thoughts as well on this on.

My thoughts on this:

DX

Javascript is just broken when it comes to throwing errors. Why? We need a way to let the consumer know the cases to be handled. I agree on the standard part but the dx of just marking a function as @throws pretty poor - you can't let the user know what errors to expect.

The user of the lib will have to always look at the documentation and keep track of changes in errors a function can produce to be able to react to them vs having a compile time information about the errors that should be handled.

I see the point of making a "throwing" version as suggested by the followup ticket in supabase but as long as lix is used internally we should focus on making a solid error handling and go for the returned error property that will Definitly help us define the error handling and communicate changes over the team (via typsaftynes).

Inter Process communication

Lix will most likely run in another process/thread/worker/deamon soon so we will have a interprocess communication going on between consumer and Excution and returning a Serializable result also including errors will be easier.


Edit 1:
My favorit - thread about this topic (Kotlin removed checked exceptions)
https://discuss.kotlinlang.org/t/in-future-could-kotlin-have-checked-exceptions/1579/74?page=4 - hint you will find arguments for the result type in favour of checked exceptions (which are also not possible in js)

@samuelstroschein
Copy link
Member

@janfjohannes @martin-lysk I opened https://linear.app/opral/issue/INBOX-60/error-handling-in-lix-and-inlang-message-sdk.

I don't understand your decision here. If we agree that compatibility issues will arise, a migration to throwing will come because users will complain as they do with supabase.

@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - fink-editor PR #2548 April 11, 2024 13:06 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - badge-service PR #2548 April 11, 2024 14:08 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - inlang-manage PR #2548 April 14, 2024 00:35 — with Render Destroyed
# Conflicts:
#	inlang/source-code/editor/src/pages/@host/@owner/@repository/State.tsx
#	inlang/source-code/manage/src/components/InlangManage.ts
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - opral-website PR #2548 April 16, 2024 13:34 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - git-proxy PR #2548 April 16, 2024 13:34 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - inlang-website PR #2548 April 16, 2024 13:34 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - inlang-manage PR #2548 April 16, 2024 13:34 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - badge-service PR #2548 April 16, 2024 13:34 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to non-reactive-errors - fink-editor PR #2548 April 16, 2024 14:19 — with Render Destroyed
@janfjohannes janfjohannes merged commit a66f1f6 into main Apr 16, 2024
3 checks passed
@janfjohannes janfjohannes deleted the non-reactive-errors branch April 16, 2024 14:25
@github-actions github-actions bot locked and limited conversation to collaborators Apr 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants