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

Transaction #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

lifeart
Copy link

@lifeart lifeart commented Jun 4, 2024

Here is sample transactional API to act like React's useOptimistic hook.

@lifeart lifeart force-pushed the transaction-poc branch 2 times, most recently from a96fb7a to 73133b2 Compare June 4, 2024 11:57
resolve(true);
}, 100);
});
await transaction.follow(asyncRequest);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo this is interesting! transactions can extend to promises!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, but I have one doubts here, likely we need to wait for all promise.then to be completed before applying transaction commit, because some logic likely already wrapped to original resolve promise

await transaction.follow(asyncRequest);
} catch (error: any) {
assert.equal(error.message, "Failed");
assert.equal(app.value, 10);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean follow implies a rollback on exception?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullVoxPopuli yep, all correct!

@lifeart lifeart marked this pull request as ready for review July 6, 2024 08:34
@lifeart lifeart changed the title Transaction POC Transaction Jul 6, 2024
@lifeart lifeart force-pushed the transaction-poc branch 2 times, most recently from cda2c92 to 8091778 Compare July 6, 2024 08:44
@lifeart
Copy link
Author

lifeart commented Jul 6, 2024

@littledan, @NullVoxPopuli it seems it make sense to throw error if some of signals, mutated in transaction is mutated outside transaction (before it's completed or rejected)

const value = signal(0);
const t = new Transaction();

t.execute(() => {
  value.set(5);
});

value.set(6); // -> Throw here, because t is active and value already consumed.

We still unable to catch case like:

const value1 = signal(0);
const value2 = signal(0);
const t = new Transaction();

t.execute(() => {
  value1.set(5);
});

value2.set(6);

t.execute(() => {
  value2.set(7);
});

But, it seems like a feature xD

@NullVoxPopuli
Copy link
Collaborator

does there need to be a way to associate signals in your transaction API? or should it work generically?

const t = new Transaction();
const value1 = t.signal(0);
const value2 = t.signal(0);


t.execute(() => {
  value1.set(5);
});

value2.set(6);

t.execute(() => {
  value2.set(7);
});

@lifeart
Copy link
Author

lifeart commented Jul 6, 2024

@NullVoxPopuli if we agree to throw error on transaction consumed signal if it's mutated outside of transaction - it's quite easy to implement. We could introduce ActiveTransactions collection (should be 1-2-3 items in same time in real apps), and check for it's signals and throw - if it's used

@lifeart
Copy link
Author

lifeart commented Jul 6, 2024

@NullVoxPopuli - 6e9be59

@ds300
Copy link

ds300 commented Jul 8, 2024

Thanks for this proof-of-concept @lifeart!

I think it'd be useful to talk about the intended semantics of a transaction API.

In databases, typically the following behaviours are guaranteed for the sake of data integrity

  1. Changes made outside of a transaction are not visible within the transaction.
  2. Changes made within the transaction are not visible outside of the transaction until it commits.
  3. Transactions fail to commit if they conflict with another transaction in some way (e.g. during the transaction a piece of data that was read or modified was changed by another transaction which committed first)

This POC makes some attempt at implementing (1) and (3), but there are gaps (happy to dig into these if you need). And (2) is explicitly not attempted.

I feel that any transaction system offered by the signals spec probably must satisfy all three of these (if not, why not? what are the usecases?). And any general-purpose userland transaction system should probably also be aiming to satisfy these guarantees unless it has very well-defined areas where inconsistency may be tolerated.

@lifeart
Copy link
Author

lifeart commented Jul 9, 2024

Hi @ds300, thank you for comments! Test cases & logic updated to satisfy 1,2,3 bullets.
Things not clear to me at the moment: Should we support reactivity inside transaction, or it's fine to trigger chain only on commit? (wondering on how we may implement optimistic ui updates based on it, and it seems we need kinda first implementation of "transaction" for it)

@ds300
Copy link

ds300 commented Jul 11, 2024

Should we support reactivity inside transaction

wondering on how we may implement optimistic ui updates based on it, and it seems we need kinda first implementation of "transaction" for it

I think if the goal is to update a view based on 'in-progress' transaction state, then it's not what I've been thinking of as a 'transaction', (i.e. code path that is isolated from the outside world to guarantee consistency). If the goal is to build a primitive that react could use to implement transitions and optimistic updates, that feels like a special higher-level concern and I don't know enough about react's needs to be giving advice here.

@lifeart
Copy link
Author

lifeart commented Sep 17, 2024

@ds300 thank you for answers! PR updated

@NullVoxPopuli
Copy link
Collaborator

@lifeart can you rebase this one? looks like CI got lost 😅

feature: transaction api

chore: transaction guards

* we don't allow cells, used in transaction to be mutated outside transaction once it's alive
* we don't allow cells, mutated during transaction (outside transaction) to be used in transaction

+

apply comments

+

+

+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants