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

feat: strongly type extended drivers #554

Closed
wants to merge 2 commits into from
Closed

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Dec 29, 2024

Currently, a driver can define its own method signatures but they will be lost thanks to DriverFactory returning a Driver rather than the original type.

This means things like the netlify driver never actually exposed their extended types (e.g. special options in getKeys).

import driver from "./netlify-blobs";

const instance = driver();

// BEFORE

instance.getKeys;
//`        ^? getKeys(base?: string, opts: GetKeysOptions)

// AFTER

instance.getKeys;
//         ^? getKeys(base?: string, tops?: GetKeysOptions & Omit<ListOptions, "prefix" | "paginate">)

With these changes, a driver can implement Driver but add its own types on top which are then exposed.

Copy link

codecov bot commented Dec 29, 2024

Codecov Report

Attention: Patch coverage is 74.87685% with 51 lines in your changes missing coverage. Please review.

Project coverage is 58.76%. Comparing base (4d61c78) to head (eaa58af).
Report is 160 commits behind head on main.

Files with missing lines Patch % Lines
src/drivers/upstash.ts 31.37% 35 Missing ⚠️
src/drivers/deno-kv.ts 84.09% 14 Missing ⚠️
src/drivers/deno-kv-node.ts 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #554      +/-   ##
==========================================
- Coverage   65.05%   58.76%   -6.30%     
==========================================
  Files          39       43       +4     
  Lines        4055     3664     -391     
  Branches      487      587     +100     
==========================================
- Hits         2638     2153     -485     
- Misses       1408     1507      +99     
+ Partials        9        4       -5     

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

Currently, a driver can define its own method signatures but they will
be lost thanks to `DriverFactory` returning a `Driver` rather than the
original type.

This means things like the netlify driver never actually exposed their
extended types (e.g. special options in `getKeys`).

With these changes, a driver can implement `Driver` but add its own
types on top which are then exposed.
@43081j
Copy link
Contributor Author

43081j commented Dec 30, 2024

I think we should do this for the repo in its current state

But given what you said in #555 , it made me wonder about what the right interface is

I wonder if in v2, we should actually have a fixed interface (like before this change). And an option which contains driver specific options, instead of merging the two

getItem("key", {
  driverOptions: {
    somethingDriverSpecific: true
  }
})

...localstorage({
windowKey: "sessionStorage",
...opts,
}),
name: DRIVER_NAME,
Copy link
Member

Choose a reason for hiding this comment

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

Reminder this another valid fix

@pi0
Copy link
Member

pi0 commented Dec 30, 2024

Appreciate your work on this PR ❤️

defineDriver<T>(): Driver is intentional at least for built-in drivers so we only assume the "common" interface only also explicit generic <T> keeps types being isolated.

In v2, it shall be a global namespace interface with explicit keys for each driver (which is, unfortunately, a breaking change for v1 as we used top-level mixed keys)

{
  ttl: 1200, // Generic
  http: { headers: { } } // driver specific
}

@43081j
Copy link
Contributor Author

43081j commented Dec 30, 2024

The netlify driver specifies it's own types which currently have no use, since we only expose a Driver

Was that intentional too? That's mostly why I did this change

e.g we specify that it can take a ListOptions but then export it as a Driver, so that information is completely lost. The only reason we're able to pass such list options is because the type Driver specifies is a Record

@pi0
Copy link
Member

pi0 commented Dec 30, 2024

Indeed, they are only usable internally now for those drivers.

Also consider, that passing driver-specific options to each transaction is something more of an escape hatch / advanced purpose. The key point of using unstorage abstraction is that an operation works regardless of the underlying driver. Only standard keys such as ttl shall be relied upon. nature of abstractions is that they are "loosy" but "consistent".

@43081j
Copy link
Contributor Author

43081j commented Dec 30, 2024

I see, it was just there to help with the internal types

And yes, I would much rather it return a Driver so there is one interface regardless of driver

Then if we must, have an option in there that is passed through to the underlying driver. Instead of changing the method signature

@pi0
Copy link
Member

pi0 commented Jan 3, 2025

(lets discuss it later)

@pi0 pi0 closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants