Skip to content

Suggestion: Narrow an argument interface when it using unpacking fields (destructuring) #20224

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

Closed
cevek opened this issue Nov 22, 2017 · 12 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@cevek
Copy link

cevek commented Nov 22, 2017

For example:

interface Foo {
    x: number;
    bar: {
        y: number;
        baz: {
            id: number;
        }
    }
}

function abc({ x, bar: { y } }: Foo) {
    return x + y;
}

abc({ x: 1, bar: { y: 2 } }); // error
abc({ x: 1, bar: { y: 2 } } as Foo); // dangerous

The problem

You need a function which can work with a complex tree object as an argument,
but it needs only a few fields from this object and you use destructuring on the argument.
And if you want to test this function you have a problem because TS requires a full Foo structure to call this function. But you know that the function don't want a full Foo object.

Today you need to write this argument interface by hands.
You write abc({}: Foo) then with autocomplete you get all fields you want
and then you replace Foo with what inside the destructuring.
It is annoying.
Plus you can't refactor your interface by renaming a field on Foo.

It will be great if TypeScript can make new interface based on a destructuring argument
and make an union type with the original argument type and this new interface.
Or maybe checker can set a flag it doesn't need to check for object literal known props

function abc({x, {bar: {y}}}: Foo)
// => 
function abc({x, {bar: {y}}}: Foo | {x: number, bar: {y: number}})

This enhancement will be very useful for writing tests, because you can write simple mocks, only what functions really needs

@mhegazy
Copy link
Contributor

mhegazy commented Nov 22, 2017

It is kinda strange that you can refactor your function, to now use baz, and you start getting errors in usage, or worse, break your public API contract...

I think desrtuctureing is just sugar for aliasing, so

function abc({ x, bar: { y } }: Foo) {
    return x + y;
}

should be have similar to:

function abc(foo: Foo) {
    let x = foo.x;
    let y = foo.bar.y;
    return x + y;
}

and in that sense is an implementation detail of the function.. chaining such an implementation detail should not change your public API contract..

i.e.

function abc({ x, bar: { y, baz: { id } } }: Foo) {
    return x + y + id;
}

should look any different to callers..

Maybe something like Pick<Foo, "x" | "bar"> would be an approximation to what your scenario needs, or may be something similar.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 22, 2017
@cevek
Copy link
Author

cevek commented Nov 22, 2017

It is kinda strange that you can refactor your function, to now use baz, and you start getting errors in usage, or worse, break your public API contract...

No, you don't break you public API contract because you specify specific interface for your argument. Checker just allow you pass more narrow type than specified one.

More practical example:

class App {
    apiKey: string;
    config = {
          urlPrefix: 'http://google.com/',
          throttling: { 
               delay: 100
          }
          // .... a lot of things 
     }      
     routes = {
          
     }
}
var app = new App();
function loadUrl({apiKey, config: {urlPrefix, throttling: {delay}}}: App) {
     // todo something with apiKey, urlPrefix and delay
}
// usual use
loadUrl(app);

// test usage
test(() => {
   var res = loadUrl({apiKey: 'abc', {config: {urlPrefix: 'mock.url', throttling: {delay: 10}}}});
   assert(res, {});
})

I think desrtuctureing is just sugar for aliasing

Yes, it is aliasing, but when you use destructuring compiler know that you use only these fields and no other one

Maybe something like Pick<Foo, "x" | "bar"> would be an approximation to what your scenario needs, or may be something similar.

I don't think that Pick can help, because it can't works with deep structures

@mhegazy
Copy link
Contributor

mhegazy commented Nov 22, 2017

No, you don't break you public API contract because you specify specific interface for your argument. Checker just allow you pass more narrow type than specified one.

not sure i follow.. if the function declaration looks like:

function abc({ x, bar: { y } }: Foo) {
    return x + y;
}

and a call of this form is legal:

abc({ x: 1, bar: { y: 2 } }); 

then you refactor your function to be:

function abc({ x, bar: { y , baz} }: Foo) {
    return x + y + baz.id;
}

now that call is an error since it does not specify the required property baz..

@cevek
Copy link
Author

cevek commented Nov 22, 2017

Yeah, you right, but when you call this function you don't know about what fields it uses, you cannot rely on what fields are required or not, you just give full object.

But when you write tests it is important to know that some fields which will be added in the future broke your tests.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 22, 2017

I understand. the issue is it not possible to relax the rule for one use case but not the other. relaxing it all the way leads to implementation details leaking into public API.

@cevek
Copy link
Author

cevek commented Nov 22, 2017

Oh, okay, I understand you. You say about barrier that protect you to use it everywhere
We can just use ! after the parameter.
Something like this:

abc({ x: 1, bar: { y: 2 } } ! ); 

@mhegazy
Copy link
Contributor

mhegazy commented Nov 22, 2017

Can u elaborate on what the proposed operator would do?

@cevek
Copy link
Author

cevek commented Nov 22, 2017

I propose to use ! to mark function parameter to allow checker use narrow inferred argument type.
Exclamation mark used in TS to mark dangerous things like:

class A {a!: number}
const a:number = null!;
interface Foo {
    x: number;
    bar: {
        y: number;
        baz: {
            id: number;
        }
    }
}
var foo: Foo;
function abc({x, bar: {y}}: Foo){return x + y}
abc(foo); // ok
abc({x: 1,  {bar: {y: 2}}}); // error
abc({x: 1,  {bar: {y: 2}}} ! ); // ok
abc({x: 1,  {bar: {y: 2}, baz: {id: 3}}} ! ); // error - excess fields
abc({x: 1,  {bar: {y: 2}, baz: {id: 3}}}); // ok
abc({x: 1,  ({bar: {y: 2}, baz: {id: 3}}} | null) ! ); // ok
abc(foo ! ); // ok
abc(null ! ); // ok

@mhegazy
Copy link
Contributor

mhegazy commented Nov 22, 2017

! today has a meaning in all these positions.. so not sure that is the right operator.. but syntax aside, I think you are looking for #2876.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 22, 2017

going back to the OP, i think the destructuring here is a red haring.. you just want to be able in your tests to pass a stripped down version of Foo to the function, based on your knowledge of the function implementation.. I think anything we do here is inherently unsafe in the face of refactoring and the correct approach is to ensure that your method contracts are simplified to be test friendly, or test your public interface using mocking.

@cevek
Copy link
Author

cevek commented Nov 22, 2017

Ok, maybe you're right. So I've figured out work today solution.

type Narrow<Super extends Sub, Sub> = Sub;

function abc(foo: Narrow<Foo, { x: number, bar: { y: number } }>) {
}

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants