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

Add documentation on transaction behavior #28

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CGamesPlay
Copy link

This was pretty confusing for me as a first-time user and I had to read the source code and do several experiments to figure this out. I'm submitting this change to the README that adds a section on the specific behavior of BEGIN, COMMIT, and REVERT and also includes a recommendation on how to use them effectively.

I also cleaned up the formatting. FYI GIthub doesn't recognize headings unless there's a space following the ###

This was pretty confusing for me as a first-time user and I had to read the source code and do several experiments to figure this out. I'm submitting this change to the README that adds a section on the specific behavior of `BEGIN`, `COMMIT`, and `REVERT` and also includes a recommendation on how to use them effectively.

I also cleaned up the formatting. FYI GIthub doesn't recognize headings unless there's a space following the `###`
@coveralls
Copy link

coveralls commented Apr 30, 2017

Coverage Status

Coverage remained the same at 92.308% when pulling 1eb7268 on CGamesPlay:patch-1 into 6868145 on mattkrick:master.

README.md Outdated
## How do optimistic actions work?

- When you dispatch a `BEGIN` action
- Your current state is recorded as the `beforeState`.
Copy link
Owner

Choose a reason for hiding this comment

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

not necessarily, this is true for the first BEGIN, but then for the 2nd BEGIN, the beforeState doesn't change

@CGamesPlay
Copy link
Author

I thought about adding this nuance to the documentation, but I think that it actually detriments from the explanatory power of the documentation by adding extra complexity. Anyone who really wants to rely on beforeState is probably doing something beyond the scope of this module anyways.

Do you have a better suggested wording, or do you think your comment is worth preventing merging this for?

@mattkrick
Copy link
Owner

my big concern is that how it works should be irrelevant. the readme should show how to use it, but the internals should be a black box where folks don't need to understand it. if they'd like to understand it, it's < 200 LOCs. I think it's perfectly fair to ask folks to read the code if they'd like to fully understand it. Then I don't have 2 sources of truth (1 in english, 1 in javascript)

@CGamesPlay
Copy link
Author

I'm inclined to agree with you, although I will point out that you reference beforeState and history in the very next section ("this will transform your state..."), so revisiting that section of the documentation to hide more of the implementation details may be worthwhile at some point in the future.

Still, I removed the implementation details from my patch and shortened the explanation to stick to "how to use redux-optimistic-ui".

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