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

Consider using less ambiguous terms when describing transforms #437

Closed
abonander opened this issue Sep 1, 2017 · 5 comments
Closed

Consider using less ambiguous terms when describing transforms #437

abonander opened this issue Sep 1, 2017 · 5 comments

Comments

@abonander
Copy link

abonander commented Sep 1, 2017

I got caught out yesterday by assuming the "inbound" transform was for going from storage -> redux state, and the "outbound" transform was for going from redux -> storage. The terminology is rather ambiguous, because it could mean going out from the Redux store to disk, or out from storage into the Redux store, and vice versa.

Maybe consider using "serialize" and "deserialize" instead, since transforms will usually be used to flatten complex datastructures to simpler ones or reinflate said datastructures from the serialized layout.

Or to be completely unambiguous, how about "toStorage" and "fromStorage"?

@rt2zz
Copy link
Owner

rt2zz commented Sep 1, 2017

agreed it is ambiguous. Serialize I dont like because transforms may do other things.

What if we just make it clearly ambiguous state and rely on the comment to guide people?

let myTransform = createTransform(
  // transform state coming from redux on its way to being serialized and stored
  (state, key) => specialSerialize(inboundState, key),
  // transform state coming from storage, on its way to be rehydrated into redux
  (state, key) => specialDeserialize(storedState, key),
  // configuration options
  {whitelist: ['specialKey']}
)

Open to other suggestions

@abonander
Copy link
Author

Sure, that's already in the README (which I'm sure you're alluding to), but if someone hits goto-definition or quick-documentation in their IDE, there's no documentation or comments to speak of. So if you're happy with the terminology, maybe just document it in the source.

@rrichardson
Copy link

My preferred bikeshed paint:
I think that the variables should indicate that they're state, but in different forms.. one is the live, and in mem state, and the other is persisted.

  (liveState, key) => specialSerialize(liveState, key),
  (persistedState, key) => specialDeserialize(persistedState, key),

the source vars should be something like :

function createTransform (serializeFn, deserializeFn, config = {})

or if you don't like serialize/deserialize ..

function createTransform (toStorageFn, fromStorageFn, config = {})

If we all like a set of words for the readme and source docs and params I am happy to put in a PR.

@rt2zz
Copy link
Owner

rt2zz commented Sep 1, 2017

gotcha, ya good idea. Right from the docs all I swapped was inboundState -> state

Couple other options might be preTransformState / postTransformState or changed createTransform to take an object of the shape { applyTransform, unapplyTransform, config }

For now I will simply add comments to source 👍

@aguynamedben
Copy link
Collaborator

Looks like this was done here: 867317c

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

4 participants