Omit is not type-safe

[samreid]

From https://github.com/phetsims/scenery-phet/issues/730#issuecomment-1125629164, we discovered that Omit does not check the type of its keys:

type TestThing = {
  name: string;
};

type OM = Omit<TestThing, 'anotehunatoheu'>; // not a type error

This problem, alternatives, lint rules to avoid using the built-in Omit type are discussed in https://github.com/microsoft/TypeScript/issues/30825

Let's discuss this at an upcoming developer meeting. But we can also take the temperature with emojis like so:

Emoji	Approximate Sentiment
👍	Let's ban the built in Omit type (via a lint rule) because it is not type safe, and use a 3rd party implementation or our own type-safe implementation. It will help us in type checking and refactoring.
👎	We should use the built-in Omit type even though it is not type safe, because it is idiomatic for TypeScript
😕	I'd like to discuss at dev meeting before forming an opinion

[samreid]

Perhaps we will implement using https://nishanths.svbtle.com/a-stricter-omit-type

[jessegreenberg]

Discussed 5/26/22 - We would like to use a third party/our own Omit that is type safe.

@samreid said that making Omit type safe may be a small change that we could add to phet-core. That would be better than pulling in a third party dependency.

1) We will add an eslint rule (probably bad-text) to find usages. 2) We will create our own Omit and then use.

The likely change to omit will be (from https://github.com/phetsims/phet-core/issues/119#issuecomment-1138924847)

type OmitStrict<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>;

from

type Omit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>;

@zepumph and @AgustinVallejo volunteered to work on this.

[zepumph]

https://github.com/microsoft/TypeScript/issues/30825#issuecomment-493605690

Shows this definition.

export type Omit<ObjectType, KeysType extends keyof ObjectType> = Pick<ObjectType, Exclude<keyof ObjectType, KeysType>>;

[samreid]

https://github.com/phetsims/phet-core/issues/119#issuecomment-1138927993 is identical to https://github.com/phetsims/phet-core/issues/119#issuecomment-1138924919 but with different type parameter names.

[zepumph]

@AgustinVallejo and I were able to make good progress on this. We used the implementation typefest. The initial find replace was quite easy:

find: \bOmit<, replace: OmitStrict<

But then we spent a long time figuring out a way to get the imports to be brought in automatically. Here is what we did.

1. Run lint-everything. This generates a report with all the file names that have errors like "OmitStrict" is definted but never used.
2. Formulate this list into a single line in webstorm "scope syntax", it basically looked like this: file[PROJECT_NAME]:scenery/nodes/Node.ts||. . . . ..
3. Create a custom scope in the find dialog and use this as the scope.
4. Find: \n\n(^import.*'((\.\.\/)+).*$) (the first import in any of those files, capturing the number of "../" for later use
5. Replace: \n\n$1\nimport OmitStrict from '$2phet-core/js/types/OmitStrict.js'; ($1 is the original import, $2 is the greedily grabbed number of ../ in the original import).
6. We found 18 sports where the first import used from './X'. Fixed manually
7. We found ~10 spots where OmitStrict was causing trouble. Only one was unable to be fixed, and has a TODO pointing to this issue (@jonathanolson it is in NodeLayoutConstraint)
8. We then added Omit to the ban-types lint rule, suggesting to use OmitStrict instead, there were no errors when we ran this new rule.

Assigning to @jonathanolson to fix that one TODO (search for https://github.com/phetsims/phet-core/issues/119). Then back to @samreid to see if there is anything else, and/or to unblock the DeepOmitStrict side issue this is blocking.

[zepumph]

@pixelzoom and @AgustinVallejo found usages in documentation that we didn't get. Yesterday we were trying to limit false positives by only looking at *.ts files, but forgot that left out documentation files. Thanks for fixing that!

[pixelzoom]

A comment on the choice of name OmitStrict ...

OmitStrict sounds like a verb + noun construction, like we're omitting something named "strict". What we actually have is adjective + noun. A name that's used in https://github.com/microsoft/TypeScript/issues/30825#issuecomment-483954049 is StrictOmit, which makes more sense to me -- a strict (adjective) version of the Omit type (noun).

I don't feel strongly that it should be changed, but I think OmitStrict is awkward -- in usage and in conversation.

[zepumph]

Sounds good to me! I'll go for StrictOmit

[zepumph]

It is now StrictOmit, a much better name! Over to @jonathanolson to finish up as said in https://github.com/phetsims/phet-core/issues/119#issuecomment-1142798208.

[jonathanolson]

Fixed that one case. @zepumph can you verify?

[zepumph]

Looks great thanks. Over to @samreid to close, and note in the DeepStrictOmit issue as needed.

[samreid]

Looks great, thanks! Closing.