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

typescript version of store #2733

Merged
merged 4 commits into from
May 15, 2019
Merged

typescript version of store #2733

merged 4 commits into from
May 15, 2019

Conversation

sanderhahn
Copy link
Contributor

Added types in the store.ts without changing any function signatures/implementation details. The rollup generates the javascript versions of the store. Added a tsd script that generates the interface definition for the store. The interface definitions are at the moment committed in the repository.

@Rich-Harris
Copy link
Member

This looks great, thank you. If the store.d.ts file is auto-generated, should it be excluded from the repo, or is it better to have it here?

@sanderhahn
Copy link
Contributor Author

The store.d.ts is derived, so it's probably better to remove the file. Wasn't sure when to generate them, so now doing it in the prepare script. Also added the cleanup for derived, but had some trouble typechecking result variable (added result as T and result as Unsubscriber). Now wondering if Unsubscriber maybe should be named Cleanup instead.

@sanderhahn
Copy link
Contributor Author

The last commits extends the ReadableStore with a get method, so that the already exposed get(store) can be executed without creating subscribe/unsubscribe closures. Can remove these commits if this is controversial. However it seems orthogonal to make readable expose a get when writable already exposes a set.

@Rich-Harris
Copy link
Member

Thanks — we've avoided having a get method up till now to keep the contract as pure as possible (otherwise it's more stuff for custom store implementers to add, and potentially makes interop with other state management libraries more difficult). It's always open for revisiting, but I reckon we should probably discuss that in a separate issue/PR

@sanderhahn
Copy link
Contributor Author

Removed the commits :)

@sanderhahn
Copy link
Contributor Author

The current implementation of get does seem to be tightly coupled to the store implementation. Because it assumes that the subscribe callback is called immediately so that it can return the value without a promise.

@sanderhahn
Copy link
Contributor Author

sanderhahn commented May 14, 2019

One problem surfaces when converting the test store.js into TypeScript. The initial value of derived is optional, however its value is passed to readable where it is required. Should the initial value of derived become required or should there be a readable that starts with an optional initial value? Maybe the situation is theoretical, but the type system flags it as a problem.

@mrkishi
Copy link
Member

mrkishi commented May 15, 2019

That's a good point. Should we even have an initial value for a derived? It gets overwritten synchronously anyway.

@Conduitry
Copy link
Member

derived is not always synchronous, thus the initial value argument which was added (pretty late along - #2397). I don't know what the answer to the type thing is though. I can't right now think of any way to address it without adding more bytes to the actual compiled js version of the store implementation or having some sort of ugly typing hack.

@sanderhahn
Copy link
Contributor Author

sanderhahn commented May 15, 2019

The current interface of derived is especially difficult for a type specification, because the source stores can have their own types (T1, ..., Tn). These types should become types of the parameters of the callback function fn: ([T1, ..., Tn]) => Tx. At the moment the TypeScript version is too simplistic and incorrectly assumes that all the types of the inputs and output are the same.

@mrkishi
Copy link
Member

mrkishi commented May 15, 2019

Although Typescript could use some extra goodness in the type-system, we actually can represent those types already:

type Stores = Readable<any> | [Readable<any>, ...Readable<any>[]]
type StoresValues<T> = T extends Readable<infer U> ? U :
	{ [K in keyof T]: T[K] extends Readable<infer U> ? U : never }

declare function derived<T, S extends Stores>(
	stores: S,
	fn: (values: StoresValues<S>) => void,
	initial_value?: T
): Readable<T>;

This will type check all combinations of derived:

const a = writable(0)
const b = writable('string')
const c = writable({ object: 1 })
const d = 0

derived(a, (a) => {})
// derived(stores: Writable<number>,
//     fn: (values: number) => void,
//     initial_value?: {}): Readable<{}>

derived([a], ([a]) => {})
// derived(stores: [Writable<number>],
//     fn: (values: [number]) => void,
//     initial_value?: {}): Readable<{}>

derived([a, b, c], ([a, b, c]) => {})
// derived(stores: [Writable<number>, Writable<string>, Writable<{object: number}>],
//     fn: (values: [number, string, {object:number}] => void,
//     initial_value?: {}): Readable<{}>

derived(a, ([a]) => {}) // errors on second argument
derived([a], ([a, b]) => {}) // errors on second argument
derived([a, b, c, d], ([a, b, c, d]) => {}) // errors on first argument, d is a number

// doesn't error but infers a = [number]
derived([a], (a) => {})

// unfortunately doesn't error, it's just a valid regular js partial
// destructuring so nothing the type system could do
derived([a, b], ([a]) => {})

(I omitted fn's full signature for brevity, but we can also differentiate fn(values, set) from fn(values) and so infer T from that in case initial_value is not used or it doesn't match)

@sanderhahn
Copy link
Contributor Author

sanderhahn commented May 15, 2019

Wow that is pretty advanced type juggling :) Removing strict checking hides the error for initial value, so maybe that is better for now.

@Rich-Harris Rich-Harris merged commit 8d539d8 into sveltejs:master May 15, 2019
@Rich-Harris
Copy link
Member

man, some of this TS stuff goes way over my head. thanks!

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.

4 participants