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

Private methods in d.ts files should be declared with rest parameters to make them more stable #10438

Closed
dbaeumer opened this issue Aug 19, 2016 · 17 comments · Fixed by #21930
Closed
Labels
Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this

Comments

@dbaeumer
Copy link
Member

TypeScript Version: 1.8.0 / nightly (2.0.0-dev.201xxxxx)

Code

export class Bug {
    private method(one: number, two: string): string {
    }
}

To make the d.ts more stable I think that the generated d.ts file should look like this:

export declare class Bug {
   private method(...params); 
}

Since the actual number of params of that method doesn't matter. All that is need is the name. This keeps the shape of a TS module more stable if private members change.

@yahiko00
Copy link

I am just wondering why the generated declaration file .d.ts even contains private members.

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug Needs More Info The issue still hasn't been fully clarified and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Aug 19, 2016
@RyanCavanaugh
Copy link
Member

@yahiko00 see #1867

@dbaeumer I don't see what the endgoal of "stability" is here. Why does it matter at all? People are going to be confused about why it's declared with a different arity than it was written with.

@yahiko00
Copy link

@RyanCavanaugh Ok I understand the purpose of letting private members in d.ts files for avoiding some weird bugs.

@dbaeumer
Copy link
Member Author

@RyanCavanaugh we use d.ts file to compute the impact of upwards compilation between modules. If the 'public' shape of a module doesn't change the modules that depend on it don't need to be recompiled. According to the discussion in #1867 names of private members belong to the public shape however not its full signature. Rest params allow to only put the minimal necessary information into the shape to achieve this.

@dbaeumer
Copy link
Member Author

A builder that would profit from this can be found here: https://github.com/Microsoft/TypeScript/tree/dbaeumer/9125

@RyanCavanaugh
Copy link
Member

Why doesn't your difference-computer simply ignore the types of private members? That's going to be a more comprehensive fix, especially because e.g. changing something from a method to an arrow function is going to show up as a difference.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2016

i agree with @RyanCavanaugh, the fix here is not to change the .d.ts emit output, but rather to optimize the public API shape differ. today it uses the .d.ts output, but there is no reason it can not be a separate output.

@dbaeumer
Copy link
Member Author

OK. I take the point of making the differ of the shape aware of this. But why does a private signature list the param names in the first place. The only reason why the private member is in is to check for overriding the name. Adding the param names is not necessary and only leaks unnecessary information into the d.t.s file.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 23, 2016

Like it or not, private isn't private at runtime. I would expect a d.ts file to properly represent the logical constructs in typescript, which would include the full definition of a method, because essentially it is only private by convention. While it maybe providing typing information for the parameters, it certainly isn't leaking anything that isn't already in the runtime emit.

@dbaeumer
Copy link
Member Author

I understand that private isn't private at runtime. This is exactly the reasoning behind my argument. I want to make it hard for other to call my private members. The more information we leak into a d.ts the easier it is.

@RyanCavanaugh
Copy link
Member

I think they have your code anyway? 😕

@dbaeumer
Copy link
Member Author

So what is the reasoning behind putting the whole signature if not necessary.

@DanielRosenwasser
Copy link
Member

Not to go off topic, but how come we even display the method signature at all? For private properties we just display any.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 24, 2016

Not to go off topic, but how come we even display the method signature at all? For private properties we just display any.

Personal opinion (as they all are) that isn't "lying", where as changing the arity of a method would be. Stripping all the type information might be logical:

export declare class Bug {
    private method(one: any, two: any): any;
}

That represents what is actually in the code without any additional information.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Needs More Info The issue still hasn't been fully clarified labels Aug 24, 2016
@dbaeumer
Copy link
Member Author

dbaeumer commented Aug 25, 2016

This is already the case. The d.t.s looks like this:

export declare class Bug {
    private method(one, two);
}

which is implicit any.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Good First Issue Well scoped, documented and has the green light and removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Sep 19, 2016
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Sep 19, 2016
@RyanCavanaugh
Copy link
Member

Accepting PRs to just emit

class Bug {
  method;
}

@Igorbek
Copy link
Contributor

Igorbek commented Sep 21, 2016

Since the effort is easy I grabbed it and made a PR #11030

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Mar 28, 2018
@mhegazy mhegazy modified the milestones: Community, TypeScript 2.9 Mar 28, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants