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

Internal aliasing #90

Merged
merged 5 commits into from
Mar 12, 2019
Merged

Internal aliasing #90

merged 5 commits into from
Mar 12, 2019

Conversation

andnp
Copy link
Owner

@andnp andnp commented Mar 11, 2019

Per this discussion, this removes some of the internal aliasing in ST. I decided to kill Keys<T> as an export too. It really is a stylistic thing and does not fit in the purview of ST in my opinion. I think ST should strive to be more focused on repetitively useful types (useful as in: can't work without it) and difficult to come up with / write types. Opinionated style choices---like Keys---don't really have as much of a place.

I decided to keep ObjectKeys as an export since it is non-obvious why we'd use the keyof any syntax. ObjectKeys should have some reasonable documentation explaining the historical relevance (which it doesn't yet), where the major value would be in the documentation instead of in the export itself. This fits the larger goal of making ST also a nice learning tool.

I just can't decide on AnyFunc. Here are the two options that I can come up with, maybe @Retsam you have a cleaner way of writing this?

export type OverwriteReturn<F extends AnyFunc, R> =
    F extends ((...x: infer T) => unknown) ? ((...x: T) => R) : never;

vs

export type OverwriteReturn<F extends (...args: any[]) => any, R> =
    F extends ((...x: infer T) => unknown) ? ((...x: T) => R) : never;

BREAKING CHANGE: No longer exporting Keys<T> either. This alias is too
opinionated stylistically and doesn't support good TS practices. If
using it, it would be best to include in a per-project basis. Besides,
it isn't difficult to write or come up with (`type Keys<T> = keyof T;`),
so is outside the scope of this project.
@andnp andnp requested a review from Retsam as a code owner March 11, 2019 20:09
@Retsam
Copy link
Collaborator

Retsam commented Mar 12, 2019

Yeah, good to keep ObjectKeys in: it's useful for learning, and makes sense in codebases where keyof any would be more mysterious to less-experienced typescript devs.

Tangent: you mentioned in the other issue being unhappy with the naming of ObjectKeys (which I rather agree with): if you wanted to change the name, you could always export it under a new name and keep the old one for backwards compatibility - if you were going to change it, it'd be good to do it before writing up more documentation. (AnyKey would be my suggestion)


For AnyFunc, are we sure that <F extends Function> isn't kosher?

export type OverwriteReturn<F extends Function, R> =
    F extends ((...x: infer T) => unknown) ? ((...x: T) => R) : never;

I had vaguely thought it wasn't kosher, but I haven't been able to break it in the Typescript playground, either. I haven't been able to turn up documentation one way or another. (Mostly because I suck at googling these things: "typescript generic extends Function", "typescript function constructor type constraint" - garbage results, but I don't know how better to phrase the query)

The only hint I've found is from the Typescript Do's and Don'ts[1], which says to never Number, String, Boolean, or Object - but doesn't forbid using Function (but I'm not sure if that's an intentional omission).

Function is definitely a lousy type, (you can't infer args or return type from it), but as far as I can tell, its a reasonable generic type constraint.

[1] I just noticed that it's "Do's and Don'ts", rather than the consistent "Dos and Don'ts" or the consistent but infuriating "Do's and Don't's". Just felt I had to share that.

@andnp
Copy link
Owner Author

andnp commented Mar 12, 2019

Oh jeez, I had entirely forgotten about Function. I can't come up with any reasonable objection to using Function in its place. I think I had simply wiped it from memory since it is useless in 99% of cases.

AnyKey sounds like a great suggestion. I'll move that over when I start beefing up the documentation more. I'll also probably start creating issues for the names of other types while I'm at it. I think it is likely that I'll have grievances with several of the names once I re-evaluate them more closely.

If you have any other names that seem off, do let me know!

@andnp
Copy link
Owner Author

andnp commented Mar 12, 2019

Also I had to read that footnote a few times to catch what was going on 🤣 I knew it had something to do with the single quote placement, but I couldn't quite put my finger on it.

@andnp andnp merged commit d95b27d into ExtendsUnknown Mar 12, 2019
@andnp andnp deleted the InternalAliasing branch March 12, 2019 17:04
@andnp
Copy link
Owner Author

andnp commented Mar 12, 2019

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@@ -24,8 +22,8 @@ export type IsNumber<T> = T extends number ? True : False;
export type IsString<T> = T extends string ? True : False;
export type IsFunction<T> =
Or<
T extends AnyFunc ? True : False,
T extends Function ? True : False>; // tslint:disable-line
T extends Function ? True : False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The find/replace made this condition redundant.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Merp. Good catch. Not sure how I missed it on my final scan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants