Refactor into TypeScript (built via tsdx for now)

[agilgur5]

• (refactor): rename localStorageAdaptor to asyncLocalStorage
• (refactor): move files into src/ to prepare to support tsdx
• (refactor): export AsyncLocalStorage as object
• (env): add standard Node .gitignore
• (env): add initial, partial tsdx support
• (types/refactor): refactor code into TypeScript
• (pkg/types): export typings and CJS builds
• (types): enable strict mode
• (types): be more specific in various places

resolves #3 (adds CJS builds via tsdx since it now requires a build process) and resolves #7 (written in TS and has typings output)

Still need to test this out and still don't have tests yet 😕 . The typings need to be tested for accuracy (is there a good way of testing this? typed tests?) and the output for equivalence with the previous non-TS version (i.e. a regression test). Could create a @types tag on NPM for folks to test it out in their apps (and perhaps to act as an alpha tag for types changes in the future?).

Some further improvements to the types could potentially be made, but not without a decent amount more work or testing:

• Easiest and most likely possible change, store can be of IStateTreeNode from MST (same type as onSnapshot's snapshot arg) I'm fairly sure. Or perhaps some other MST type. Or maybe even object would be more specific and still accurate
• IAsyncStorage can be made more generic (c.f. https://github.com/localForage/localForage/blob/master/typings/localforage.d.ts or perhaps even more generic?) and then exported and used as the storage type (or one of its types, see below) and re-exported outside of the package as its typings for others to use.
• storage needs crazier typing or no default (require importing the adaptor like with redux-persist) because IAsyncStorage | Storage doesn't work. It needs to support window.localStorage as an argument, otherwise only IAsyncStorage type. And all will behave like IAsyncStorage after the if check. Pretty sure there's some TS metaprogramming like that which MST does to make this possible, but as a complete TS newbie, I have no clue and am not sure where to look in MST source code.

[agilgur5]

Some typings and tsconfig questions that I'm not sure of as a TS newbie and not knowing how to test that the types are accurate. Also potentially more that could be changed in tsconfig but idk.

The IAsyncStorage rename should probably be done to avoid future confusion. Otherwise looks fine, the total diff isn't that big

[agilgur5]

I didn't add tslib to my devDeps but I do use ...rest and ...spread in some places... not sure if this can be removed or if I should add tslib or if this is fine 🤷‍♂

[agilgur5]

There's an _extends helper added to the compiled code as well as code around rest/spread. I also checked and saw that tslib was in my node_modules already and it's actually a dependency of tsdx, so maybe that part of the default package.json it creates is outdated?

[agilgur5]

Ehhh confused again because per the tslib docs, the output code should import tslib if this is true.... why isn't it??

[agilgur5]

Ah so to handle differences in Union Types, TS uses type guards for similar to effect to pattern-matching. Though it looks like these will have runtime performance impact as they require a runtime check (might be possible to hide some redundant ones in prod builds, though they might be necessary for safety)

[agilgur5]

Other than the tslib stuff, this looks good to go to me now. Can always fix typings later if needed and can probably do most typings changes in patch releases too, so should be low pressure.

I also manually checked the compiled output to make sure it's equivalent so no immediate need for regression testing (though tests would still be good to have ofc #4)

[agilgur5]

woops ^that commit reference is a mistake, referenced the wrong issue by number

[agilgur5]

@rafamel since it sounded like you were using TS in your project, would you like to review this? Would be good to have a second pair of eyes :)

[rafamel]

Great job!! Everything looks good. I also gave it a run within a ts project project and there are no typing issues popping up either. The only thing is, it looks like you're supporting synchronous functions for storage via callWithPromise, so I'd modify the typings to allow for that and also make it explicit.

[rafamel]

Sorry about that, I assumed a couple things and only took a sideways read at the code. I initially thought that IAsyncLocalStorage applied to IOptions.storage. Forget about what I said, though I would type IOptions.storage :)

[agilgur5]

Awesome, thanks for the quick check and test inside a TS project @rafamel!

Per my initial comment/description, there's a decent bit of work to type IOptions.storage:

1. it supports a generic AsyncStorage-like interface. This is something more generic than IAsyncLocalStorage (IAsyncLocalStorage should extend it) that can have other properties via [key: any]: any, returns object | string from getItem, and Promise<T> from the rest since doesn't matter if the rest aren't void. It technically doesn't need to support clear or removeItem since those aren't used internally (but it should ofc).
   But I'm not even sure if the interface shape I'm describing there is actually fully compatible with localForage's types since it also supports things like callbacks. Might have to add something like ...args: any[] to all the keys to that interface in order to support localForage. And then maybe a test to ensure the types support localForage permanently (which I guess is just a test to see if the intersection exists?).
2. it supports passing localStorage as an argument (causing the AsyncLocalStorage adaptor to be used). The latter means it would be of type IAsyncStorage | Storage so I'd need to typeguard against Storage and then like throw if it's not specifically localStorage as that's the only sync Storage engine that's currently supported out-of-the-box.
   Alternatively, can make a breaking change so that there's no default storage mechanism and that one must import and pass AsyncLocalStorage as an argument if they want to use it (similar to redux-persist's current behavior).

Should be made more specific at some point but the work required to handle Storage and figure out localForage means I'm gonna backlog it for now 😅

[rafamel]

Sounds good, though just as further info, this would work w/ localforage:

interface IStorage {
  clear(): Promise<void>;
  getItem(key: string): Promise<string>;
  removeItem(key: string): Promise<void>;
  setItem(key: string, value: string): Promise<void | string>;
}

As callbacks are optional, localforage's object conforms w/ the interface. The only change is setItem (it returns the input type). Regarding localStorage, it won't as it's synchronous, so i'd suggest:

interface IStorage {
  clear(): Promise<void> | void;
  getItem(key: string): Promise<string | void | null> | string | void | null;
  removeItem(key: string): Promise<void> | void;
  setItem(key: string, value: string): Promise<void | string> | void | string;
}

Then we'd only need to make AsyncLocalStorage a function taking the storage object and returning the safe promisified version. So at index, instead of:

  if (typeof window.localStorage !== 'undefined' && (!storage || storage === window.localStorage)) {
    storage = AsyncLocalStorage
  }

We'd have:

  if (!storage && typeof window.localStorage === 'undefined') {
    throw Error(`...error`);
  }
  const asyncStorage = makeAsyncStorage(storage || window.localStorage);

The only thing left would be objects instead of string + jsonify. For this I'd suggest making jsonify an argument and writing a persist overload that only allows it to be false when storage supports taking and returning objects (we'd have to interface it).

[agilgur5]

Thanks for the suggestions @rafamel! 😄

As callbacks are optional, localforage's object conforms w/ the interface. The only change is setItem (it returns the input type).

For reference, how did you test this to make sure? Just seeing if the intersection compiles? Because I'd like to add a test for that eventually and this being effectively my first usage of TS, still got some things to learn.

Regarding localStorage, it won't as it's synchronous, so i'd suggest:

Yep that adds a decent bit of complexity as I was thinking. I thought about having some AsyncStorage class that would have the same effect as your makeAsyncStorage function when I first created this library, but I'm not sure if anyone would even use something other than localStorage as sync storage (build it when someone asks!). Though I guess it doesn't seem like significantly more work if I'm going to already add typings for localStorage.

In any case, I'm going to merge this as is for now, and will figure out the storage interfaces later. Kinda tired of working on it for now 😅

---

Now I'm still wondering if it's best practice to have the types as a minor version bump or a patch bump... It adds new exported goodies like types and CJS (minor) but has not made any actual changes to the API surface, which remains the same (patch) - so could go both ways imo. Probably doesn't matter much anyway 🤷‍♂

[rafamel]

Regarding tests, they can be done by writing a normal ts file using your exported functions and running tsc --noEmit, provided your root tsconfig.json includes both the src and the folder you write your tests at. Otherwise, you can create a test tsconfig as follows:

tsconfig.test.json:

{
  "$schema": "http://json.schemastore.org/tsconfig",
  "extends": "./tsconfig.json",
  "include": ["src/**/*", "test/**/*"],
  "compilerOptions": {
    "baseUrl": "./",
    "noEmit": true
  }
}

Then just run: tsc --project tsconfig.test.json

As for minor vs. patch release, I'd personally do minor as I'd count this as a feat, though not like it'd matter too much being still on 0.x :P

Hope it helps!

[agilgur5]

Sorry, by tests I meant specifically how did you make sure "localforage's object conforms w/ the interface"?

[rafamel]

Yes, that's how. Run tsc passing the noEmit flag either via command or a tsconfig project file, provided the tsconfig includes both the source and the file you're doing persist(a, b, { storage: forage }); at.

[agilgur5]

persist(a, b, { storage: forage });

This was the part I was curious about, thanks! I was thinking there might be some other way to handle checking whether a type conforms to another in TS

[agilgur5]

This PR was released in v0.1.0