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 Transforms #16

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add Transforms #16

wants to merge 9 commits into from

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Nov 10, 2019

  • 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.
  • 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.

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
Copy link

codecov-io commented Nov 19, 2019

Codecov Report

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

Impacted file tree graph

@@          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.

Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

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.

src/index.ts Outdated Show resolved Hide resolved
src/transforms/index.ts Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
- 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
- so that these can work as examples and so that their order is very
  explicit as they're treated like any other transform internally

- create new transforms/ directory that will hold all built-in
  transforms moving forward
  - and export them in transforms/index.ts
- more importantly, makes the logic a little simpler / easier to read
- describe usage, link to MST snapshots, show typings, and link to
  internal examples of whitelist and blacklist transforms
  - use commit permalink for internal transforms so it won't 404 or
    change over time (though ofc will need to be updated if the
    transforms API changes)
- describe ordering and show a small diagram of it end-to-end
- more because we added whitespace and split up functionality, not
  because transforms are in any way a big code addition (~15 LoC)
- the array passed into the transforms should be required
  - same for arrToDict function
  - they're now called only when array is defined, and I think that
    behavior is more intuitive -- it should error if the array isn't
    defined
    - (test): in internal usage, they're never undefined, so this was
      reducing coverage because it was dead code
    - if the transforms were to eventually be externally visible /
      import-able, they should definitely throw an error in that case
      too -- if external users pass undefined to it
      - and the typings change means it'll happen at design-time too
      - definitely think it's more intuitive for external users this
        way too

- (docs): use this commit for w/blist transform examples
  - or a variant of this commit -- just one where arr is required
- yay back to 100% code coverage now!
  - there's a transform branch missing bc there's no test case where
    there's something in storage and at least one toStorage-only
    transform
    - but covering this else branch doesn't really matter

- create some transform fixtures
- abstract out a setItem function

- should consider splitting the tests and fixtures into separate files
  soon, esp now that we have a few different test suites here
- not necessarily the most useful transforms there, but they are
  examples nonetheless
  - and they show both to and from Storage usage
@agilgur5
Copy link
Owner Author

agilgur5 commented Dec 12, 2019

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 🤔 🤔

- now docs are roughly same size as source
  - and README is getting a bit unwieldy, thinking about splitting the
    Transform docs into their own file
@danielgek
Copy link

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

@agilgur5 agilgur5 added progress: external blocker This is blocked by an issue in an external dependency kind: feature New feature or request labels Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request progress: external blocker This is blocked by an issue in an external dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants