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

should transforms be mutable? #328

Closed
ianstormtaylor opened this issue Sep 15, 2016 · 2 comments
Closed

should transforms be mutable? #328

ianstormtaylor opened this issue Sep 15, 2016 · 2 comments

Comments

@ianstormtaylor
Copy link
Owner

ianstormtaylor commented Sep 15, 2016

@Soreine rightly brought up this point a few times.

My initial reasoning from an old issue:

For mutability, my thought was that since transforms are expected to be continuously chained together, it would make more sense to have them be mutable and save performance on all of the interim steps required to get from start to end. I could be convinced this is a bad idea though!

But at the same time this is the only mutable data in the whole library, so maybe it's worth a few extra transform = calls as long as it's not a performance concern...

I'm really not sure what the right decision is here. Open to arguments either way!

@Soreine
Copy link
Collaborator

Soreine commented Sep 16, 2016

Here what I have noticed in practice:

Immutable

  • ✅ No noticeable performance issue so far
  • ❌ Every step that requires to access an intermediate state need to transform.apply() the whole transform so far. Why the whole transform ? Because this transform.apply() must not be carried over (e.g. transform.apply().state.transform() an continue), since it would mess with the undo/redo logic. For undo/redo, it's best if .apply() is used to delimit the whole logical transformations only.
  • ✅ If performance becomes an issue, it's worth having a look at Record.asMutable and Record.asImmutable. Core transforms with lot of operations could use that. Custom transforms could use it as well, as long as they know they don't call functions on the Transform that rely on its immutable nature to do some logic (for example, doing a operation just to inspect the potential result, then continue with the initial transform).

Mutable

  • ✅ No need to transform.apply() to inspect new state.
  • ✅ If we ever need a copy, we can still transform.state.transform(), right ?
  • ❌ Disconcerting at first, since the rest of the library is immutable

After writing this, I'd say we can stick to mutable transforms, because I don't see any drawback, past the initial surprise.

@ianstormtaylor
Copy link
Owner Author

Closing for now, although I agree that it's not perfect, so happy to revisit later if we come up with something better!

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

No branches or pull requests

2 participants