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

fix: export types needed for defining custom stores #7358

Closed
wants to merge 1 commit into from

Conversation

vrde
Copy link

@vrde vrde commented Mar 10, 2022

I wrote a custom store to have a more flexible derived store. The function signatures looks like:

export function derivedWritable<S extends Stores, T>(
  stores: S,
  fn: (values: StoresValues<S>, set: (value: T) => void) => Unsubscriber | void,
  initial_value?: T
): Writable<T> { ... }

Both Stores and StoresValues are not exported in svelte/store. I had to copy paste the type definition from this file and duplicate code. Unless I'm using TypeScript wrong (I'm still quite new to it), I think these types plus few others should be exported.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

I wrote a custom store to have a more flexible `derived` store. The function signatures looks like:

```typescript
export function derivedWritable<S extends Stores, T>(
  stores: S,
  fn: (values: StoresValues<S>, set: (value: T) => void) => Unsubscriber | void,
  initial_value?: T
): Writable<T> { ... }
```

Both `Stores` and `StoresValues` are not exported in `svelte/store`. I had to copy paste the type definition from this file and duplicate code. Unless I'm using TypeScript wrong (I'm still quite new to it), I think these types plus few others should be exported.
@vrde vrde changed the title Export types needed for defining custom stores [fix] Export types needed for defining custom stores Mar 10, 2022
@ryanatkn
Copy link
Contributor

ryanatkn commented Mar 20, 2022

Related, I want to use the types SpringOpts and SpringUpdateOpts from svelte/motion but they're not exported.

I'm using this workaround for the spring module to avoid copy/pasting, but this strategy does not work for all of the types exported in this PR:

type SpringOpts = Exclude<Parameters<typeof spring>[1], undefined>;
type SpringUpdateOpts = Exclude<Parameters<ReturnType<typeof spring>['update']>[1], undefined>;

My apologies if this is the wrong place for this. In my case it's a trivial issue.

@vrde
Copy link
Author

vrde commented Apr 14, 2022

👋 Any updates? The PR is ready, all tests pass, is there anything else left? Thanks!

@vrde
Copy link
Author

vrde commented Aug 17, 2022

@baseballyama @bluwy: would it make sense to update this PR to export also the types mentioned by @ryanatkn? Wondering if there are more types that should be exported 🤔.

PS: sorry for the direct mention, just wanted to move this forward if possible ❤️

@jrmoynihan
Copy link

Just came to make this same PR. I keep having to manually add export directives each time I update the package.

@dummdidumm
Copy link
Member

AFAIK @Conduitry had reservations of exposing the Invalidator type because strictly speaking that's internal API - though since it's available in other public types, that's kind of a lost cause.

@benmccann benmccann changed the title [fix] Export types needed for defining custom stores fix: export types needed for defining custom stores Mar 14, 2023
@dummdidumm dummdidumm added this to the one day milestone Mar 20, 2023
@vrde
Copy link
Author

vrde commented Jun 5, 2023

AFAIK @Conduitry had reservations of exposing the Invalidator type because strictly speaking that's internal API - though since it's available in other public types, that's kind of a lost cause.

Not sure why you say it's a lost cause. You have devs that are asking for these types to be exported. That's a clear need. I'm not sure why this is not part of the discussion.

@vrde
Copy link
Author

vrde commented Jul 2, 2024

Closing this PR because in the meantime I moved to react 😂

@vrde vrde closed this Jul 2, 2024
@vrde vrde deleted the patch-1 branch July 2, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants