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

Move shared logic into AbstractReconcileTransaction to avoid mistakes #7567

Conversation

millermedeiros
Copy link
Contributor

not the first time that someone forgets to implement checkpoint and rollback on a ReconcileTransaction (see #7558 and #6689) so I decided it was worth moving the shared logic into an AbstractReconcileTransaction

I kinda followed the same pattern used on Transaction.js (export a Mixin property) but in this case a plain object would probably be fine.

Note that the ReactNativeReconcileTransaction was also missing the checkpoint and rollback, not sure if intentional or not (not familiar with the React codebase).

PS: maybe someone have a better name for it and/or knows a better folder to place it.

/cc @spicyj

@sophiebits
Copy link
Collaborator

I appreciate this but I don't think I'm going to take this. It is quite plausible that another transaction wrapper would need a checkpoint and creating this "superclass" makes it more likely that someone will forget to override the checkpoint/rollback methods, even though you could argue the current solution is worse when the methods aren't implemented.

ReactNativeReconcileTransaction should implement checkpoint/rollback though.

@millermedeiros
Copy link
Contributor Author

follow up #7619

@millermedeiros millermedeiros deleted the abstractreconciletransaction branch August 31, 2016 05:35
@millermedeiros millermedeiros restored the abstractreconciletransaction branch August 31, 2016 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants