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 #9

Open
ggascoigne opened this issue Jul 6, 2021 · 8 comments
Open

typescript #9

ggascoigne opened this issue Jul 6, 2021 · 8 comments

Comments

@ggascoigne
Copy link

First let me say that this is a fantastic library, and I really appreciate you making it available.

I've been poking at this with typescript and I was wondering what you level of typescript interest is? To use this library seamlessly with typescript, we need type declarations. There are several ways to handle this.

Firstly you need types, that would be an index.d.ts file like this:

export default class Zet<T> extends Set<T> {
    static union<T>(...sets: Set<T>[]): Zet<T>;
    static intersection<T>(...sets: Set<T>[]): Zet<T>;
    static difference<T>(...sets: Set<T>[]): Zet<T>;
    static symmetricDifference<T>(setA?: Zet<T>, setB?: Zet<T>): Zet<T>;
    static subset<T>(setA: Set<T>, setB: Set<T>): boolean;
    static superset<T>(setA: Set<T>, setB: Set<T>): boolean;
    static map<T>(set: Set<T>, func: (value: T, index?: number, array?: T[]) => unknown): Zet<unknown>;
    static filter<T>(set: Set<T>, func: (value: T, index?: number, array?: T[]) => unknown): Zet<T>;
    static reduce<T>(set: Set<T>, func: (previousValue: T, currentValue: T, currentIndex: number, array: T[]) => any, initializer?: any): T;
    union(...sets: Set<T>[]): Zet<T>;
    intersection(...sets: Set<T>[]): Zet<T>;
    difference(...sets: Set<T>[]): Zet<T>;
    symmetricDifference(other?: Zet<T>): Zet<T>;
    subset(other: Set<T>): boolean;
    superset(other: Set<T>): boolean;
    map(func: (value: T, index?: number, array?: T[]) => unknown): Zet<unknown>;
    filter(func: (value: T, index?: number, array?: T[]) => unknown): Zet<T>;
    reduce(func: (previousValue: T, currentValue: T, currentIndex: number, array: T[]) => any, initializer?: any): T;
}

There are a number of ways to handle this.

  1. We could just add this to this repo and add them to the build. I'll admit that I don't know rollup and completely failed to work out how to get this added to the build correctly.
  2. I could submit the types to DefinintelyTyped - this is the main repo for third party types. This requires no changes in this repo, but adds another dependency for library users.
  3. I've already ported the whole library over to typescript, mostly because I was curious and it helped me understand the code better. It was pretty quick and painless, having good tests helped immensely here. If you were interested I could just submit a PR for all of this, but since it's quite the change, I figured I'd see if you were interested in this before pushing ahead.
  4. I could just create a separate ts-zet library. Honestly I'm not sure if this is worth it. It does make maintaining the types trivial, since they end out being in lockstep with the code, and any signature changes there (likely around the generics) would just flow though, but since there's no functional change, and honestly, very little code change, I don't know if it adds any value.

FYI the only significant change in the code other than adding types was replacing the spread operator with Array.from since typescript doesn't support spreading Sets.

Thoughts?

@codingedgar
Copy link

I think 2 over 4, specially since there is no answer

@AHBruns
Copy link

AHBruns commented Jul 21, 2022

Surprised this doesn't have types yet. Has the author indicated they don't want other's to supply types? If not, I'd be happy to add types to Definitely Typed on this library's behalf.

@ggascoigne
Copy link
Author

yeah, I completely dropped the ball on this. I do think that example that I included earlier is a valid set of types to use (and have been using them internally in my project for a year or so), I just got sidetracked before I could get the tests put together for the types that would be desired/required for DefinitelyTyped.

If you, @AHBruns, fancy putting that together, I'll happily review it for you. If not, I can try and get to it later, maybe this weekend.

@AHBruns
Copy link

AHBruns commented Jul 21, 2022

There are a few things I'd change, for example, the static intersection should require at least 2 set arguments, and the instance intersection should require at least 1 set argument. Same with difference. Not sure about union since we could say the union of no sets is the empty set and the union of 1 set as itself. Also the map should probably map to a generic type, not unknown.

Mostly minor though. If you have the types written, I'll defer to you. Ping me when you need a review. If not, I can probably get around to writing them sometime this weekend.

@ggascoigne
Copy link
Author

I see what you mean, but since the underlying code doesn't make that requirement, I didn't change it. I literally just put types on the existing interface.

@AHBruns
Copy link

AHBruns commented Jul 21, 2022

Actually looking at the implementation, there's a quite a few optimizations that could be made. I may just write something myself. I'd still be happy to help with reviewing typings, of course, but for my use-case I'll probably just write something new. Will release it, with types, if it ends up being as useful as Zet.

@AHBruns
Copy link

AHBruns commented Jul 21, 2022

Also, the implementation for difference does require at least 1 set. It will encounter a runtime error if you give it 0 sets.

@AHBruns
Copy link

AHBruns commented Jul 21, 2022

It handles 1 by just returning the empty set though.

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

No branches or pull requests

3 participants