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

The types aren't compatible with CommonJS #1137

Closed
2 tasks done
Tracked by #1684
aduth opened this issue Mar 27, 2020 · 22 comments
Closed
2 tasks done
Tracked by #1684

The types aren't compatible with CommonJS #1137

aduth opened this issue Mar 27, 2020 · 22 comments
Labels
bug Something does not work as it should ✭ help wanted ✭

Comments

@aduth
Copy link

aduth commented Mar 27, 2020

Describe the bug

  • Node.js version: v12.13.0
  • OS & version: macOS Catalina 10.15.3

Standard usage of the default export got as a callable expression yields a type error in TypeScript "This expression is not callable".

Possible factors include:

  • Type-checking JavaScript code
  • Importing using CommonJS

I observe in the generated types...

  • The default export is import("./create").Got
  • Got is interface extends Record, not a function (source)

Possibly the types generated are not able to correctly detect type from the creation of the default export here:

const got = create(defaults);

Actual behavior

index.js:5:25 - error TS2349: This expression is not callable.
  Type 'typeof import("/Users/andrew/Desktop/got-bug/node_modules/got/dist/source/index")' has no call signatures.

5     console.log( (await got(ISS, { json: true })).body.iss_position );
                          ~~~


Found 1 error.

Expected behavior

(No type errors)

Code to reproduce

package.json

{
  "name": "got-bug",
  "version": "1.0.0",
  "scripts": {
    "test": "tsc --allowJs --checkJs --noEmit index.js"
  },
  "devDependencies": {
    "typescript": "^3.8.3"
  },
  "dependencies": {
    "got": "^10.7.0"
  }
}

index.js

(Code based on the Runkit example. Note: The current example appears to be broken, based on json: true needing to be responseType: 'json')

const got = require("got");
const ISS = "http://api.open-notify.org/iss-now.json";

(async() => {
    console.log( (await got(ISS, { responseType: 'json' })).body.iss_position );
})();

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@szmarczak
Copy link
Collaborator

#1072

@szmarczak
Copy link
Collaborator

Got is interface extends Record, not a function (source)

You forgot that it also extends GotRequestMethod, which is a function.

@szmarczak
Copy link
Collaborator

What TypeScript version are you using?

@aduth
Copy link
Author

aduth commented Mar 30, 2020

Got is interface extends Record, not a function (source)

You forgot that it also extends GotRequestMethod, which is a function.

Oops, right you are! I did miss that.

What TypeScript version are you using?

I am using the latest version of both Got (10.7.0) and TypeScript (3.8.3).

Outside the default configuration, I've tried a few additional options to see if anything helps, to no avail unfortunately (--esModuleInterop, --allowSyntheticDefaultImports, --moduleResolution).

Based on the error message and what else I've been able to find of it, not sure if it might have something to do with it being unable to match any of the signatures of GotRequestMethod, especially with untyped / duck typed JavaScript. I tried a few variations, again with no luck:

  1. Revise GotRequestMethod to define a very liberal call signature.
  2. Revise the JavaScript implementation to (a) only pass string and (b) define variable type explicitly as string /** @type {string} */ ( 'https://...' ).

To be clear, I'm just throwing ideas out there based on my attempts to resolve this. Not sure if any of it is applicable. To me, it seems like the minimal code example in the original comment should be sufficient to work without errors.

@szmarczak
Copy link
Collaborator

Please set up an example repository.

@ties-s
Copy link

ties-s commented Mar 30, 2020

Running into this issue as well

@szmarczak
Copy link
Collaborator

@ties-s

Please set up an example repository.

I cannot reproduce this.

@aduth
Copy link
Author

aduth commented Mar 31, 2020

@szmarczak Would a CodeSandbox demo work?

https://codesandbox.io/s/js-typescript-got-demo-qirz5

Ignoring the preview, and waiting a moment for the dependencies to load, you should see the inline error when got is invoked:

image

@aduth
Copy link
Author

aduth commented Apr 1, 2020

Also not sure if this is related, but I was encountering a very similar error when using an older version of the chalk package (2.4.x) which was resolved by upgrading to the latest version (3.0.0). I tracked it back to the changes at chalk/chalk#344 and a related comment at sindresorhus/meta#9 (comment) . May be a pattern to follow there?

@szmarczak
Copy link
Collaborator

@aduth Why are you trying to run Got in a browser sandbox? Got doesn't support browsers.

@szmarczak
Copy link
Collaborator

Please set up a GitHub respository so I can just clone and look what's wrong.

@szmarczak
Copy link
Collaborator

Please try replacing

export default got;

// For CommonJS default export support
module.exports = got;
module.exports.default = got;

export * from './create';
export * from './as-promise';

with

export = got;

then recompile Got and try again.

@szmarczak
Copy link
Collaborator

Also why are you running tsc on a non-TypeScript file?

@Caerbannog
Copy link

Also why are you running tsc on a non-TypeScript file?

This is useful when migrating a large javascript codebase to typescript bit-by-bit with the allowJs option of tsc.

@aduth
Copy link
Author

aduth commented Apr 1, 2020

I've pushed a repository including a minimal reproducible case here:

https://github.com/aduth/tmp-got-types-demo

Reproduce by:

  1. npm install
  2. npm test

@aduth Why are you trying to run Got in a browser sandbox? Got doesn't support browsers.

CodeSandbox can interpret the TypeScript configuration, which is what I'd intended to demonstrate with the inline error reporting. The runtime of the code itself isn't particularly relevant for the problem.

Also why are you running tsc on a non-TypeScript file?

As @Caerbannog mentioned, it's a supported feature of TypeScript:

https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html

Please try replacing [...] then recompile Got and try again.

I can check this later and report back with the results.

@Caerbannog
Copy link

As a workaround I migrated to typescript the few .js files in my project that used got, and everything is now working fine.
It's not a general solution for bigger projects, though.

@szmarczak
Copy link
Collaborator

I can check this later and report back with the results

That'd be awesome.

@aduth
Copy link
Author

aduth commented Apr 1, 2020

Please try replacing

I had some success with this, though the changes as proposed weren't quite as easy to implement. I'm not sure if code you referenced is from a version older than that of the current master, but it presently looks like this for exported members in index.ts:

got/source/index.ts

Lines 117 to 161 in 74d66b2

export default got;
// For CommonJS default export support
module.exports = got;
module.exports.default = got;
// Export types
export * from './types';
export {
Got,
GotStream,
ReturnStream,
GotRequestMethod,
GotReturn
} from './create';
export {
ProxyStream as ResponseStream
} from './as-stream';
export {
GotError,
CacheError,
RequestError,
ReadError,
ParseError,
HTTPError,
MaxRedirectsError,
UnsupportedProtocolError,
TimeoutError,
CancelError
} from './errors';
export {
InitHook,
BeforeRequestHook,
BeforeRedirectHook,
BeforeRetryHook,
BeforeErrorHook,
AfterResponseHook,
HookType,
Hooks,
HookEvent
} from './known-hook-events';

Ultimately, it did seem to be resolved by using export = got;, but since export = can not be alongside other export statements, and those other export can't be removed directly (build will fail), I had to create a new entrypoint whose sole contents were:

import got from './index-original';
export = got;

This built fine, fixed the error in my JavaScript project, and provided the type hints I'd hope for.

I can open a pull request if you'd like, or leave it to you or another maintainer, as I expect it might need some further thought on how these files are structured if there needs to be the one entrypoint with one export =.

@aduth
Copy link
Author

aduth commented Apr 1, 2020

As a workaround I migrated to typescript the few .js files in my project that used got, and everything is now working fine.
It's not a general solution for bigger projects, though.

Aside: I use the TypeScript tooling in quite a few of my JavaScript projects, with no plans to adopt TypeScript the language. I'd consider it a perfectly valid use-case, in addition to the scenario you describe.

@szmarczak szmarczak changed the title TypeScript Types: Default export "This expression is not callable" (CommonJS, JavaScript) The types aren't compatible with CommonJS Apr 2, 2020
@szmarczak szmarczak added the bug Something does not work as it should label Apr 2, 2020
@szmarczak szmarczak added this to the Got v12 milestone May 4, 2020
@sindresorhus sindresorhus removed this from the Got v12 milestone Jan 5, 2021
@eaviles
Copy link

eaviles commented Feb 9, 2021

Having the same issue but when trying to call got.extend on a JS codebase. I ended up overriding the type definition shipped with the module.

@eaviles
Copy link

eaviles commented Feb 11, 2021

Having the same issue but when trying to call got.extend on a JS codebase. I ended up overriding the type definition shipped with the module.

By the way, I managed to get around this by doing:

const { default: got } = require('got');
got.extend({});

@szmarczak szmarczak mentioned this issue Mar 21, 2021
71 tasks
@szmarczak szmarczak mentioned this issue Apr 13, 2021
13 tasks
@szmarczak
Copy link
Collaborator

Fixed in #1667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

6 participants