feat(drivers): add type generics to support strongly typed options of drivers

[schplitt]

Implements support for typed options from driver. Resolves #660

So I think I better explain what is going on here.

I added an optional type argument for the drivers to declare their option types (get, set, remove, has, getKeys).
This type argument is passed to the Storage (via the driver) where the actual type is then extracted and passed onto the methods.
It achives strongly typed options in the methods, for example get from the specific driver used.

I tried to deduce what kind of options are needed in what method, but please double check them in the Driver and Storage types.

As a side note, there is no type safety for the options inside the createStorage function, though as we currently always (as far as I tell) just pass them right back into the driver, there should be no issues.

As part of this I had to add these types to the netlify-blob driver. There you can see the strongly typed options for the set, get and getKeys in action.

From here we can go into each driver and set their option types to get strong type support in their option methods.

I hope my explanation makes sense.
Please take a look.
If you have any questions, please feel free to ask right away!

[pi0]

Thanks for the PR and explanations.

The main benefit of unstorage is that it abstracts away the implementation of drivers with a unified (same) interface. Adding per-driver extensions takes that benefit away, and is no better than directly using each driver's underlying client (it might be even more powerful and better typed!)

Other than options for creating a driver, their low-level types should not vary.

I would like to instead shift towards introducing vendor namespaced options like { netlify: { ... } + generic ones like { tll }.

We could try adding these extensions via a global shared interface in a non breaking change way.

[schplitt]

Thanks for the PR and explanations.

The main benefit of unstorage is that it abstracts away the implementation of drivers with a unified (same) interface. Adding per-driver extensions takes that benefit away, and is no better than directly using each driver's underlying client (it might be even more powerful and better typed!)

Other than options for creating a driver, their low-level types should not vary.

I would like to instead shift towards introducing vendor namespaced options like { netlify: { ... } + generic ones like { tll }.

We could try adding these extensions via a global shared interface in a non breaking change way.

I understand where you are coming from with not wanting to have divergent options in driver to keep a unified way.
Though currently the drivers already have different options (netlifyblobs, http) which would result in a breaking change with your proposal, as we would need to remove them from the root options object and nest them.

Though currently we have the behaiviour that some users are using per-driver extensions and will run different runtime behaiviours when switching driver. They cannot tell that this specifc option is not supported by the new driver as the options are currently typed as Record<string, any>, which is even worse than having a non unifed API.

Although having an api like `get(key: string, commonOptions?: { ttl }, driverSpecificOptions?: { ... } } might be best to allow for a clear driver specific opt in whilst keeping the common options seperated. (But tbh it's just a different flavor to your suggestion)

Atleast in my head we can not really work around a breaking change if we want to type the options.

I would like to typed options achived in some way. How would you like to proceed?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.41%. Comparing base (70310f9) to head (3e83105).
⚠️ Report is 41 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   59.99%   62.41%   +2.41%     
==========================================
  Files          42       43       +1     
  Lines        3657     3887     +230     
  Branches      590      660      +70     
==========================================
+ Hits         2194     2426     +232     
+ Misses       1460     1458       -2     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pi0]

For now, I think we should not touch Storage/createStorage instances to inherit anything from mounted drivers, as that change would be non-goal, and really hard to break after. I know you were mainly looking for this, but sorry :( Internal driver types were mainly for development.

---

The best first step would be to contribute to add vendor namespaced optional options to the global TransactionOptions interface (currently it is fully untyped on the global level!)

And next, we could allow precise types for the Storage interface to Pick/Omit vendor namespace keys using a generic map of mounts to their types.

LMK if you like to take over these 😊

[schplitt]

Could you elaborate a bit on what you had in mind with the Picking and Omitting?

For me it sounds similar to what I came up with,but using the e.g. supported flags + driver name to later on pick/omit out of the global type. Difference being the source type not coming from the driver itself but from picking out of the global TrransactionOptions with the specific types being registered in the global TransactionOptions type as well

To have type safety in the Storage methods this way we would still need to pass along the driver type into the storage to allow for type inference.

An example of your idea would be very helpful!

Anyway, I would like to help with that 😄

[pi0]

Roughly what I have in mind:

// [per driver]

export interface XYZOptions {}

export interface XYZDriver {
  setOptions: {};
  getOptions: {};
}

// [_drivers.ts (auto gen)]

export type BuiltinDrivers = {
  xyz: XYZDriver;
};

export type GetOptions = {
  xyz?: XYZDriver["getOptions"];
};

export type SetOptions = {
  xyz?: XYZDriver["setOptions"];
};

// [types.ts]

interface TransactionOptions {
  ttl?: number;
}

interface StorageDefinition {
  items: unknown;
  mounts: Record<string, { driver: keyof BuiltinDrivers }>; // <-- step 2
}

interface Storage<T> {
  getItem: (key: string, opts?: TransactionOptions & GetOptions) => any;
  setItem: (
    key: string,
    value: any,
    opts?: TransactionOptions & SetOptions
  ) => void;
}

• 1. Extracting get/set options as individual interfaces from each driver and auto-generating is a must-have that we are missing today (note that it is not tied to internal storage definition, but a public interface)
• 2. TypedStorage and inferring the mount from the key could be done in the next steps to replace Get/SetOptions with an inferred subset from only the known vendor key. But please note it is really tricky, as currently we are maintaining a backward compatible StorageItem interface until the next major, and the benefit would be lower and requires users to explicitly type mount points as vendor drivers only to not have auto-complete on disalloweed keys (which could be a disatantage also sometimes, if you want flexibility to pass several vendor options and be able to swap them out!)

[schplitt]

I think I got the idea

It's a 2 step process and for now only implement step 1 until the old storage interface is removed .

I think I can work something out.

I have a bit of an issue with the TransactionOptions having ttl.

A: this implies all drivers support it which is wrong.
B: get having ttl also makes no sense.

[pi0]

this implies all drivers support it which is wrong.

We should make it optional type + JSDocs to mention only some drivers support it

get having ttl also makes no sense.

Yeah my bad it should be only on set, so not on transactionOptions 😆