Skip to content

Unable to augment exported function from a module with a overload signature #21566

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
patsissons opened this issue Feb 2, 2018 · 19 comments
Closed
Labels
External Relates to another program, environment, or user action which we cannot control.

Comments

@patsissons
Copy link

TypeScript Version: 2.7.1

Search Terms: augment module function export

Code

// imports.ts
export function test(arg: number): any;
export function test(arg: number): any {
  return arg.toString();
}

// augmentations.ts
import { test } from './imports';

declare module './imports' {
  function test(arg: string): any; //  TS2384: Overload signatures must all be ambient or non-ambient.
}

test('foo');

Expected behavior: test function from imports.ts is augmented with overloaded string signature.

Actual behavior: TS2384 Error

I tried to hunt down this issue as I thought it previously existed but after numerous pages of other unrelated issues I gave up the hunt. I believe this issue has been around since the beginning of module augmentations, and perhaps it is by design. I suspect the problem is that there is no way to define a static module function as a module augmentation.

I have run into use cases where this ability would be very useful. Typically when a particular library lags behind in updating their type declarations, but deficiencies in the typings are not declared within a class or interface scope. The solution is typically to just hold back on updating typescript, until the library is properly updated (which can sometimes be long wait depending on how active the library is).

@patsissons
Copy link
Author

A work around for this issue has been determined to work for typescript 2.6.x+ by using // @ts-ignore above the augmentation line. This will tell the compiler to ignore the issue, but the typing system will still merge the overloaded signature.

@SalathielGenese
Copy link

SalathielGenese commented Feb 2, 2018

I don't think this should be reported on TS... Rather at your 'ix/iterable/concat' package repository, following our discussion on TypeScript gitter

As far as I see, such cases SHOULD NOT be handle by TS but the author of your dependency package should fix the signature there.

@patsissons
Copy link
Author

there will be a separate pull request for the ix module, their typings are incorrect and need to be patched. This issue is to address the inability to augment static function exports from a module.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

I am not seeing this error locally, what am i missing?

c:\test\21566>type a.ts
import { test } from './a';

declare module './a' {
  function test(arg: string): any; //  TS2384: Overload signatures must all be ambient or non-ambient.
}

test('foo');

c:\test\21566>type b.ts
export function test(arg: number): any;
export function test(arg: number): any {
  return arg.toString();
}

c:\test\21566>tsc --v
Version 2.7.1

c:\test\21566>tsc

c:\test\21566>echo %ERRORLEVEL%
0

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Feb 2, 2018
@patsissons
Copy link
Author

@mhegazy do you have those type commands reversed?

$ cat imports.ts
export function test(arg: number): any;
export function test(arg: number): any {
  return arg.toString();
}

$ cat augmentations.ts
import { test } from './imports';

declare module './imports' {
  function test(arg: string): any; //  TS2384: Overload signatures must all be ambient or non-ambient.
}

test('foo');

$ ./node_modules/.bin/tsc --v
Version 2.7.1

$ ./node_modules/.bin/tsc augmentations.ts
src/types/augmentations.ts(4,12): error TS2384: Overload signatures must all be ambient or non-ambient.
src/types/augmentations.ts(4,12): error TS2394: Overload signature is not compatible with function implementation.

@mhegazy mhegazy added Bug A bug in TypeScript and removed Needs More Info The issue still hasn't been fully clarified labels Feb 2, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

ah sorry, but that seems right to me.. augmentations is meant to change the behavior of something you can not control, e.g. a library you are importing, but if you control your function, why not add an overload to it?

@patsissons
Copy link
Author

Yea, sorry if it wasn't clear originally. The example is just proof of bug, in the use case I encountered this bug i don't have direct control of the source that requires the augmentation.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

So was it in a .ts file or in in a.d.ts file?

@patsissons
Copy link
Author

it is a .ts file, but that's only because the project has a very strange setup and exposes its types through the ts source. But this is all for a module fetched via npm.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 3, 2018

Then that is a bug for the declaration file really. I would recommend filing this on https://github.com/ReactiveX/IxJS/issues instead.

@mhegazy mhegazy added External Relates to another program, environment, or user action which we cannot control. and removed Bug A bug in TypeScript labels Feb 3, 2018
@patsissons
Copy link
Author

patsissons commented Feb 3, 2018

i think they are two separate issues. a broken module with bad type declarations, and the bug with the typescript compiler that prevents generates a compiler error when augmenting static function exports.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 3, 2018

The idea here is to avoid undesired merges that might lead to surprises. and the rule is all overloads must look the same, e.g. can not have one exported and one not, similarly, can not have one ambient and one not.. again the rational is , if you are implementing the function, how can some other entity tell you what parameter types your function supposed to handle..

Faulty declaration files, are just that, faulty declaration files, and the right fix is to fix them not to patch over them.

@patsissons
Copy link
Author

i agree with your statements, but what happens when fixing them isn't possible (not that this is the case here). It just seems strange for this case given that if the compiler ignores the issue, the typing system is perfectly happy with the augmentation. Additionally, there is currently (afaik) no way to augment a static exported function from a module, i'm guessing your argument here is that you should never need to given proper type declarations.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 3, 2018

i'm guessing your argument here is that you should never need to given proper type declarations.

not exactelly. I can disable the check.. the question is what is the value of the check. we originally added it to avoid surprises when you have a function with the same name as a global/imported function that you do not know is merging with your function. removing the check makes your scenario work, but also makes that issue go unchecked.
I am arguing that fixing one declaration package and keeping the check in place, in aggregate, has higher value for all language users.

@patsissons
Copy link
Author

yea that makes sense. My argument is more about not limiting ability to monkey patch dependencies that have gone stale and don't receive timely updates. Maybe the real answer here is to simply use the @ts-ignore directive for the few cases where this is relevant, as I am currently doing.

@SalathielGenese
Copy link

SalathielGenese commented Feb 4, 2018

I think this issue must be closed at this stage of discussion, since... TypeScript is extremly reluctant prudent when it comes to bug-prone features.

The problem is real, removing it is error-prone. Thanks, //@ts-ignore can be a workaround.

@patsissons
Copy link
Author

@mhegazy feel free to close this at your leisure, I think we have come to a consensus for now.

@SalathielGenese
Copy link

SalathielGenese commented Feb 5, 2018

The External label is really appropriate ? Since the issue have two aspects of which only one is external

@pvieira91
Copy link

pvieira91 commented Feb 28, 2018

I came across this problem too.
Basically, I'have a module helpers.ts that has a bunch of utility methods to reduce the boilerplate of redux.
One of this modules has the following signature:

export function createReducer<S, M = {}>(
  initialState: S,
  mapping: { [type: string]: (state: S, payload: any, meta: M) => S }
): any

Then, I created a redux-middleware which injects a meta data to actions. When using that middleware I want to overload the createReducer definition to specify the generic M. That way, every other module which uses the createReducer method will be given with a more specific type definition.

declare module "./helpers" {
  function createReducer<S, M extends ActionMeta = ActionMeta>(
    initialState: S,
    mapping: { [type: string]: (state: S, payload: any, meta: M) => S }
  ): any;
}

By doing that I came across the ts error TS2384: Overload signatures must all be ambient or non-ambient.

It's true that I can control that helpers module, since it's a ts file in my source code, however, ideally, I want to keep that file agnostic so that, in the future, i could share among others projects.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
External Relates to another program, environment, or user action which we cannot control.
Projects
None yet
Development

No branches or pull requests

5 participants