Add Transforms

[agilgur5]

• object with toStorage and fromStorage functions
  • toStorage is called after removing whitelist and blacklists and
    before serializing to JSON
  • fromStorage is called after deserializing from JSON
• refactor whitelist and blacklist into transforms

Review Notes

• transforms needs to have a default arg, even if it's later guaranteed to not be undefined, so the || []is currently redundant. This might be related to Object is possibly 'undefined' microsoft/TypeScript#29642.
  • In the process of writing a comment for that issue, I realized that behavior is probably correct / expected. As the onSnapshot callback is async, transforms could be set to undefined before it runs. While it's not in our code, the typings allow that, vs. having a default ensures it can't be set to undefined (it's no longer optional/?). See also my playground link.
• Recursive import of ITransform from ./index seems non-optimal
  • Created a transforms/ directory and moved transforms there. In the future, would like users to import from mst-persist/transforms, so it might make sense to only export ITransform etc from there, and not from mst-persist. Would have to figure out how to make an alias with tsdx.
• The new transform docs are quite verbose / wordy. I think the interface code block and the ordering visual explain much better than the words (though I am not sure if they are accessible), so maybe it make sense to remove all the verbal/words description. The words might actually make it more confusing than just the code block and visual alone.
  • Actually, upon looking at it again at a later date, I do think the verbal description provides some extra details that are helpful. Like the Transform portion links to the MST snapshot docs and describes the object as "snapshot-like". The Ordering portion describes what the functions listed in the text actually do (which may not be so clear to beginners).
• Tests would be very useful to ensure there are no regressions with this. And as we add more to the API and proceed to thinking about migrations, it becomes even more important that there aren't regressions. As bugs here could potentially cause users to lose data, tests are particularly critical for this library.
  • As of Add Tests w/ 100% Code Coverage #21, mst-persist now has tests and 100% code coverage. This PR now just needs to be rebased and add some tests.
    • Done, tests added here.
• Need to write up an RFC for "Transforms & Migrations (& Plugins??)" to consider how future uses of transforms will look like so we make more iterations now instead of later when they will be breaking changes.
  • Done, see [RFC] Transforms & Migrations (& Plugins?) #23

Trade-off Analysis

Went through a few iterations of this API and source code. Making whitelists and blacklists internal transforms gave me a bit of hands-on usage as I was building it too, which was good. Thinking of removing the whitelist and blacklist config options in the next major/breaking version and just telling users to use the transform as it simplifies the config options API and makes the ordering explicit.
I considered adding a transformOrder config option that (after some considerations) would just be an array of enums/strings with a default of ['whitelist', 'blacklist', 'customs'] for full control of the built-in options, but honestly just telling users to use them as transforms themselves is much simpler and less confusing.

Used toStorage and fromStorage as it makes very explicit the directionality, unlike redux-persist's "inbound" and "outbound" which are very confusing / ambiguous imo (outbound from what??).
I decided to use those names a while ago, but when I was recently looking up the rationale for redux-persist's transform design in PRs and issues, I also found this issue asking for the naming to be toStorage and fromStorage as well (that probably would've been breaking at that point in time though). Here's another comment responding to a very confused PR about the same naming issue. These are just two instances of public confusion I found, there's probably more public instances, and far more private instances.

I also don't believe we need to use a createTransform function like redux-persist has, which adds more imports and I think makes the API more weighty. redux-persist has some different constraints specific to redux, namely immutability and reducers to deal with. Some of the naming concerns seem due to reducer usage as well.

Considered using reduce and reduceRight like redux-persist, but decided against limiting it that way or making it seem more immutable (it's not and doesn't have to be). Should still return a snapshot-like object right now, but could in theory remove that in the future or add ability to return other things. I think focusing on the snapshot makes it narrow and easier to understand for now. Reducing it does make it more explicit that one transform's output is another's input. 🤷‍♂ guess it's probably not that big a difference which to use.

Attempted writing the JSON (de)serialization as a transform too, but it doesn't fit the Transform Interface as it's to and from a string, and because the deserialization can return early and bail if falsey. The latter is particularly weird and accounting for it would make the interface quite confusing (unless we just threw/caught an error?).

Future Work

It might make sense to make JSON (de)serialization and other features available as "plugins", with "transforms" being one type of plugin. This would allow for full control of ordering of the entire internals and basically inverts the paradigm for full flexibility (kind of like how webpack does it). Though at that point, the source code isn't much beyond onSnapshot and applySnapshot, though we do add a lot of built-in functionality and smart defaults.

See #23

[codecov-io]

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #16   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      6    +4     
  Lines          44     60   +16     
  Branches       13     16    +3     
=====================================
+ Hits           44     60   +16

Impacted Files	Coverage Δ
src/transforms/whitelist.ts	100% <100%> (ø)
src/index.ts	100% <100%> (ø)	⬆️
src/transforms/utils.ts	100% <100%> (ø)
src/transforms/blacklist.ts	100% <100%> (ø)
src/transforms/index.ts	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebddd6a...a117d91. Read the comment docs.

[agilgur5]

Some minor things to change, otherwise looks good after all the iterations.

If we decide to export ITransform only from mst-persist/transforms, then there should really be an example of end-to-end usage of importing ITransform and persist and using them together to implement a transform in the README directly.

[agilgur5]

Importing from mst-persist/transforms is currently blocked on jaredpalmer/tsdx#175 / jaredpalmer/tsdx#365 as tsdx seems to have a bug with multiple inputs where it sets all their output filenames to the same thing 😕

175 is the bug, 365 is how to solve that bug, which requires some sort of configuration because naming and directory structure is not necessarily clear. I was able to create a workaround for this in jaredpalmer/tsdx#175 (comment) though

Would still have to make the second entry in src/, i.e. a src/transforms.ts that just does export * from './transforms/index' so we don't have to import from a subdirectory.

EDIT: Added a PR to support multiple entries: jaredpalmer/tsdx#367. There is still the issue that 365 points out, that it wouldn't be mst-persist/transforms, but mst-persist/dist/transforms. Not horrible, but if we support that usage then we can't easily break that usage later.
The alternative is to add a root-level entry, i.e. a transforms.js that does export * from './dist/transforms' and add it to files, but that syntax is ESM, so we'd have to add a CJS version too etc etc etc. And on top of that, we'd need a secondary package.json to support module vs main 😖 😖
Although... if we added transforms/package.json, add only that to files, and just had that point to ../dist/transforms.js in main and ../dist/transforms.esm.js in module that might just work 🤔 🤔

[danielgek]

Hey, @agilgur5 im really interested on this one, is there something i can do to help getting this merged ? i can see there were some updates on tsdx side, but not sure if it still blocks this