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 2.8 — initial support & updates #14

Merged
merged 7 commits into from
Apr 23, 2018

Conversation

fbartho
Copy link
Contributor

@fbartho fbartho commented Apr 3, 2018

A quick stab at importing some of the new best ways to implement these helper functions using TypeScript 2.8 features.

  • Removes discouraged Diff<…> helper from README and document suggestion of using Exclude
  • Removes now-public NonNullable implementation from README, and document that it's now globally available
  • Update implementations of Omit and Overwrite based on community & MS recommendations

Unfortunately, to test this (today), you have to manually patch definitelytyped-header-parser in your node_modules directory, as it doesn't yet support TypeScript 2.8 (via a constant) and because of this dtslint can't test the code. If you do patch that to allow TypeScript 2.8, then the tests pass.

Fixes #13

@fbartho
Copy link
Contributor Author

fbartho commented Apr 3, 2018

As documented above, the tests is expected to fail in CI until definitelytyped-header-parser updates via: microsoft/definitelytyped-header-parser#12 which can then permit dtslint to update via microsoft/dtslint#113

types/index.d.ts Outdated
* @see https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-307871458
* TypeScript Documentation suggests that as of 2.8 you should use the built-in "Exclude" instead.
*
* @see https://github.com/Microsoft/TypeScript-Handbook/blame/master/pages/release%20notes/TypeScript%202.8.md#L245
Copy link
Owner

Choose a reason for hiding this comment

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

Let's either alias Diff to Exclude or just take it out; I think I'd be fine with the latter and bumping the major version since we're already making the breaking change of requiring TypeScript 2.8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently making it an Alias, but happy to delete it instead, please advise!

Copy link
Owner

Choose a reason for hiding this comment

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

Let's delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted!

types/index.d.ts Outdated
*
* @see https://github.com/Microsoft/TypeScript/issues/15012#issuecomment-346499713
* Deprecated: available in TypeScript Core as of 2.8
* @see https://github.com/Microsoft/TypeScript-Handbook/blame/master/pages/release%20notes/TypeScript%202.8.md#L203
Copy link
Owner

Choose a reason for hiding this comment

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

Let's just take it out.

types/index.d.ts Outdated
*/
export type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>;
export type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>;

/**
* Find the overlapping variants between two string unions.
Copy link
Owner

Choose a reason for hiding this comment

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

Can we replace Overlap with Extract?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with Overlap or the internals of Extract yet, happy to do whatever you suggest!

Copy link
Owner

Choose a reason for hiding this comment

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

/**
 * Extract from T those types that are assignable to U
 */
type Extract<T, U> = T extends U ? T : never;

Diff and Overlap are dual, as Exclude and Extract are dual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! So what's the specific action you would like here? Sounds like the answer to your first question is "Yes", but do you want me to just delete the Overlap implementation from type-zoo?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think Diff and Overlap have been superseded by Exclude and Extract respectively.

Copy link
Owner

Choose a reason for hiding this comment

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

So let's delete both Diff and Overlap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted!

types/index.d.ts Outdated
*/
export type Overwrite<T, U> = Omit<T, Overlap<keyof T, keyof U>> & U;
export type Overwrite<T, U> = {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see why this one needs to change except to use Extract instead of Overlap:

export type Overwrite<T, U> = Omit<T, Extract<keyof T, keyof U>> & U;

@fbartho
Copy link
Contributor Author

fbartho commented Apr 3, 2018

Initially actionable changes addressed, happy to make more!

@pelotom
Copy link
Owner

pelotom commented Apr 3, 2018

Thanks, this is looking good on paper (although I haven't tried using it). Thanks also for tackling the dtslint issue, hopefully that's fixed soon and we can get this merged!

@fbartho
Copy link
Contributor Author

fbartho commented Apr 3, 2018

If you want to test it early, the patch is in node_modules/definitelytyped-header-parser/index.js

-     TypeScriptVersion.all = ["2.0", "2.1", "2.2", "2.3", "2.4", "2.5", "2.6"];
+    TypeScriptVersion.all = ["2.0", "2.1", "2.2", "2.3", "2.4", "2.5", "2.6", "2.7", "2.8"];
// And later down
        "ts2.6",
+        "ts2.7",
+        "ts2.8",
        "latest",

@fbartho
Copy link
Contributor Author

fbartho commented Apr 23, 2018

@pelotom Woooo! It's working!

types/index.d.ts Outdated
@@ -31,7 +14,7 @@ export type Overlap<T extends string, U extends string> = Diff<T, Diff<T, U>>;
* @see https://github.com/pelotom/type-zoo/issues/2
* @see https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-307871458
*/
export type Overwrite<T, U> = Omit<T, Overlap<keyof T, keyof U>> & U;
export type Overwrite<T, U> = Omit<T, Extract<keyof T, keyof U>> & U;
Copy link
Owner

@pelotom pelotom Apr 23, 2018

Choose a reason for hiding this comment

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

Come to think of it, this could be (and always could have been) just

export type Overwrite<T, U> = Omit<T, keyof T & keyof U> & U;

There was never any need for the old Overlap, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I don't know -- I didn't do TypeScript when that existed, does that pass the old tests? -- Do you want it changed to this?

I've never personally used the Overwrite operator, but your solution looks totally like an obvious implementation. I would probably have implemented it that way if I needed that operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, looking at my code, I have exactly that pattern, but I don't do the keyof T & keyof U and just target the Omit directly at the keys I want to junk & replace.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, thinking it through I'm pretty sure Extract<keyof T, keyof U> is equivalent in all cases to keyof T & keyof U, so let's make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

@pelotom pelotom left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@pelotom pelotom merged commit 1399936 into pelotom:master Apr 23, 2018
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