microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
99.43k stars 12.32k forks source link

Allow extra fields in object literals if they are used in methods on the object #50467

Open RedBeard0531 opened 1 year ago

RedBeard0531 commented 1 year ago

Suggestion

There are two issues with the type inference applied to object literals with methods in argument position:

  1. Methods have their this type implicitly set to the inferred parameter type, rather than the actual type of the object literal (as they do in other contexts)
  2. Additional fields are flagged as errors even if they are used by the methods

The default behavior of not allowing unknown fields in object literals is useful in general because it detects typos in optional field names. However, it can be a problem if you want to add extra private(ish) data shared between methods on an object. I think that TS should not consider extra fields as "unknown" if they are used by methods on the object.

There are currently two workarounds (as shown in the code sample and playground link below): 1) pull the object literal out to a named const, or 2) make the function that accepts the object a template rather than giving it the exact interface type. These both have the downside that they completely turn off unknown field checking, so you can still get typos in optional parameters. I think that an ideal solution would still only permit "known" fields, it should just expand the definition of what fields are known.

🔍 Search Terms

I searched for "object literal methods" and poked around a bit, but didn't see anything like this. It is similar to "Contextual typing of methods and functions in object literals" in this PR, in that I think that change may have caused at least some of this.

✅ Viability Checklist

My suggestion meets these guidelines:

⭐ Suggestion

I think your issue template has duplicate "Suggestion" sections...

📃 Motivating Example

Playground link

interface Methods {
    foo: (n: number) => void;
}
function withMethods(methods: Methods) {
    methods.foo(1)
}

// This works fine, although type of `n` must be specified explicitly
const myMethods = {
    isBoundCorrectly: true,
    foo(n: number) {
        console.log(this.isBoundCorrectly);
                            // ^?
    }
}
withMethods(myMethods)

// This doesn't, even though it is doing the exact same thing.
withMethods({
    isBoundCorrectly: true,
    foo(n: number) {
        console.log(this.isBoundCorrectly);
                            // ^?
    }
})

// Making the function a template fixes it, but seems like it shouldn't be necessary.
// Also, you need to do so for every argument, possibly recursively.
// And it may not be an option when consuming someone else's API.
// Finally, it is too weak in that it allows completely unknown fields.
function withMethodsTemplate<T extends Methods>(methods: T) {
    methods.foo(1)
}
withMethodsTemplate({
    isBoundCorrectly: true,
    foo(n) {
     // ^?
        console.log(this.isBoundCorrectly);
                            // ^?
    },
    asdf: true,
})

💻 Use Cases

This seems useful any time you want to pass an object literal with methods to a function.

RedBeard0531 commented 1 year ago

Here is the actual code that caused me to run into this.

// This needs to be a separate const rather than an inline argument due to
// https://github.com/microsoft/TypeScript/issues/50467.
const bindingContext = {
  isBoundCorrectly: true,
  didChange(realm: Realm) {
    assert(this.isBoundCorrectly);
    console.log("in didChange()", realm.currentTransactionVersion);
  },
  beforeNotify(realm: Realm) {
    assert(this.isBoundCorrectly);
    console.log("in beforeNotify()", realm.currentTransactionVersion);
  },
};
Helpers.setBindingContext(realm, bindingContext);

Interestingly, this is in a test of a native module generator to ensure that it is correctly invoking methods on objects with the correct this context.

The definition for BindingContext is generated as

export type BindingContext = {
  didChange?: (r: Realm) => void;
  beforeNotify?: (r: Realm) => void;
  schemaDidChange?: (r: Realm) => void;
};