search

googleapis / nodejs-logging-bunyan

Node.js client integration between Stackdriver Logging and Bunyan.
https://cloud.google.com/logging/
Apache License 2.0
63 stars 34 forks source link

TS2507 Error for @google-cloud/logging dependency on 0.9.5 #241

Closed anandnimkar closed 5 years ago

anandnimkar commented 5 years ago

Getting the following Typescript build error on the lastest version of this library:

[Project Root]/node_modules/@google-cloud/logging/node_modules/google-gax/build/src/streaming.d.ts(47,42): error TS2507: Type '{ default: DuplexifyConstructor; obj(writable?: Writable | undefined, readable?: Readable | undefined, streamOptions?: DuplexOptions | undefined): Duplexify; }' is not a constructor function type.

Environment details

Steps to reproduce

  1. run tsc from command line
  2. observe error: node_modules/@google-cloud/logging/node_modules/google-gax/build/src/streaming.d.ts:47:42 - error TS2507: Type '{ default: DuplexifyConstructor; obj(writable?: Writable | undefined, readable?: Readable | undefined, streamOptions?: DuplexOptions | undefined): Duplexify; }' is not a constructor function type.

Thanks!

ofrobots commented 5 years ago

/cc @JustinBeckwith @alexander-fenster

JustinBeckwith commented 5 years ago

Greetings! I am having trouble reproducing this one. Can you try a few things?

  1. rm -rf node_modules package-lock.json
  2. npm install
  3. tsc

If that doesn't work - can you share your full package.json?

anandnimkar commented 5 years ago

That didn't work. Tried with both yarn and npm.

{
    "name": "sample",
    "version": "0.1.0",
    "description": "sample",
    "license": "UNLICENSED",
    "dependencies": {
        "@google-cloud/debug-agent": "^3.0.0",
        "@google-cloud/logging-bunyan": "^0.9.4",
        "@google-cloud/trace-agent": "^3.1.1",
        "awesome-phonenumber": "^2.2.6",
        "body-parser": "^1.18.3",
        "cldr-data": "^32.0.1",
        "deepmerge": "^2.2.1",
        "dotenv": "^6.0.0",
        "express": "^4.16.3",
        "geolib": "^2.0.24",
        "globalize": "^1.4.0",
        "grpc": "^1.15.1",
        "lodash": "^4.17.10",
        "longjohn": "^0.2.12",
        "mathjs": "^5.0.4",
        "moment": "^2.22.2",
        "moment-timezone": "^0.5.21",
        "monet": "^0.8.10",
        "on-finished": "^2.3.0",
        "on-headers": "^1.0.1",
        "request": "^2.87.0",
        "request-promise": "^4.2.2",
        "yargs": "^12.0.2"
    },
    "devDependencies": {
        "@types/actions-on-google": "^1.10.0",
        "@types/bunyan": "^1.8.4",
        "@types/dialogflow": "^0.6.3",
        "@types/dotenv": "^4.0.3",
        "@types/globalize": "^0.0.34",
        "@types/jest": "^23.3.10",
        "@types/lodash": "^4.14.109",
        "@types/mathjs": "^4.4.1",
        "@types/moment-timezone": "^0.5.9",
        "@types/on-finished": "^2.3.1",
        "@types/on-headers": "^1.0.0",
        "@types/papaparse": "^4.5.5",
        "@types/request-promise": "^4.1.41",
        "archiver": "^2.1.1",
        "bottleneck": "^2.13.2",
        "bunyan": "^1.8.12",
        "concurrently": "^3.5.1",
        "decompress": "^4.2.0",
        "del": "^3.0.0",
        "dialogflow": "^0.6.0",
        "fs-extra": "^7.0.0",
        "gulp": "^4.0.0",
        "gulp-sourcemaps": "^2.6.4",
        "gulp-typescript": "^5.0.0-alpha.3",
        "handlebars": "^4.0.11",
        "husky": "^0.14.3",
        "jest": "^23.1.0",
        "jest-html-reporter": "^2.4.2",
        "merge2": "^1.2.2",
        "nodemon": "^1.17.5",
        "papaparse": "^4.6.2",
        "pump": "^3.0.0",
        "ts-jest": "^23.10.5",
        "ts-node": "^6.1.1",
        "tslint": "^5.10.0",
        "typescript": "^3",
        "uuid": "^3.3.2"
    }
}
JustinBeckwith commented 5 years ago

Woah, this is getting weird. I took your package.json, and did a few things:

After that... everything works for me :scratches head: To show you what I'm doing, I put mah code here: https://github.com/JustinBeckwith/gettingweird

Can you clone that, npm install, and npm run compile?

anandnimkar commented 5 years ago

So I spent some time trying to reproduce this with your repo above, and the issue is my tsconfig.json file. Try this file:

{
    "compilerOptions": {
        "module": "commonjs",
        "esModuleInterop": true,
        "moduleResolution": "node",
        "target": "es6",
        "outDir": "dist",
        "declaration": false,
        "sourceMap": true,
        "strict": true,
        "baseUrl": ".",
        "resolveJsonModule": true
    },
    "exclude": [
        "node_modules",
        "dist"
    ]
}
zamiang commented 5 years ago

Have the same issue. Happy to help triage as well. Aside from some project specific include / exclude settings, my ts-config is effectively the same (esModuleInterop, moduleResolution, target, strict…etc all the same)

zamiang commented 5 years ago

I get the error when updating from 4.1.1 to 4.2.0 if that helps.

DominicKramer commented 5 years ago

@JustinBeckwith I've assigned this issue to you since it looks like you have good insight into the problem. If you don't have time to address this issue, just let me know. Thanks.

JustinBeckwith commented 5 years ago

I actually don't know what's happening 😢 I tried to come up with a repo above, but haven't been able to figure it out. If you could take it, that would be awesome.

zamiang commented 5 years ago

This comment is a bit of a WIP for identifying the underlying issue.

In short, this issue is really weird. I manage two projects that use this library, and the error appears in one and NOT the other one. I believe the tsconfigs are the same in both but I will 100% verify.

Project A node modules with google related packages:

"@google-cloud/error-reporting": "0.6.0",
"@google-cloud/logging": "4.3.0",
"@google-cloud/storage": "2.4.1",
"google-auth-library": "3.0.1",
"googleapis": "37.1.0",
$ npm list google-gax
project-a@0.0.1
└─┬ @google-cloud/logging@4.3.0
  └── google-gax@0.25.0

Ts Error

node_modules/google-gax/build/src/streaming.d.ts:47:42 - error TS2507: Type '{ default: DuplexifyConstructor; obj(writable?: Writable | undefined, readable?: Readable | undefined, streamOptions?: DuplexOptions | undefined): Duplexify; }' is not a constructor function type.

47 export declare class StreamProxy extends Duplexify {

Project B: node modules with google related packages:

  "@google-cloud/error-reporting": "0.6.0",
  "@google-cloud/logging": "4.3.0",
$ npm list google-gax
project-b@0.0.1
└─┬ @google-cloud/logging@4.3.0
  └── google-gax@0.25.0

No type error

jordanworner commented 5 years ago

Is the issue due to this commit https://github.com/googleapis/gax-nodejs/commit/45ce64408761eea662b9eeec3ae1edec1fa396f4#diff-c07e52dfa4ea9cdea96f9e152666f9da

and the corresponding tsconfig-google.json update in gts https://github.com/google/ts-style/commit/d92f876627a167b0c436d091253f591447133d73#diff-19e170b73889957f2d2c69de5a7a5d7a , the gax tsconfig extends this one.

The error is the way duplexify is imported in streaming.d.ts, Duplexify is exported as a default but "allowSyntheticDefaultImports": true was removed from the gts tsconfig. The issue seems to be the default value for allowSyntheticDefaultImports, it will be true if module === "system" or --esModuleInterop is set and module is not es2015/esnext

So if you have 

"module": "commonjs",
"esModuleInterop": true,

allowSyntheticDefaultImports will evaluate to true and cause the compile error.

zamiang commented 5 years ago

Do google libraries generally intend to depend on allowSyntheticDefaultImports being turned off for consumers of the libraries? (oddly enough, the 'apollo' assorted libraries depend on esModuleInterop being ON…which is why it is enabled for the project which has this issue - https://github.com/apollographql/apollo-server/issues/1977 though I am unclear whether that is intentional on their part)

DominicKramer commented 5 years ago

My worry is if there exists an example of some-app that depends on packageA and packageB where packageA has a particular allowSyntheticDefaultImports/esModuleInterop configuration and packageB has another allowSyntheticDefaultImports/esModuleInterop configuration such that any tsconfig configuration for some-app breaks either using packageA or packageB.

I'm trying to come up with a minimal example to see if the root cause is a Typescript problem, a Google gax problem, or a problem in this repo. The scenarios I have come up with so far all work fine.

As a short-term solution, to get compilation to work, one can use const {LoggingBunyan} = require('@google-cloud/logging-bunyan'); to temporarily ignore the Ttypechecking errors until we identify the root cause.

lostpebble commented 5 years ago

Do google libraries generally intend to depend on allowSyntheticDefaultImports being turned off for consumers of the libraries? (oddly enough, the 'apollo' assorted libraries depend on esModuleInterop being ON…which is why it is enabled for the project which has this issue - apollographql/apollo-server#1977 though I am unclear whether that is intentional on their part)

Consumers of libraries should not be forced in to any kind of tsconfig.json setting - as these modules don't exist in a vacuum. For example, I have now hit a complete wall with updating my Google Cloud dependencies because of this issue. I use allowSyntheticDefaultImports: true and esModuleInterop: true, and this cannot be changed because of the massive amounts of other code that depends on them.

This feels like a configuration issue on the @google-cloud/* library's side, as consumers should not be getting type errors for the internal workings of libraries they are importing (type errors completely out of the consumer's control).

carnun commented 5 years ago

Are there any updates on the issue? @jordanworner definitely seems to have identified the breaking change, i.e. in my case changing streaming.d.ts back to import Duplexify from 'duplexify' resolves the compiler error.

DominicKramer commented 5 years ago

@carnun The problem with changing streaming.d.ts is that there are two options:

import Duplexify from 'duplexify';

or

import * as Duplexify from 'duplexify';

However, with the settings

"module": "commonjs",
"esModuleInterop": true,

then one of the options fails to compile but with

"module": "commonjs",
"esModuleInterop": false,

then the other fails to compile. Thus just changing streaming.d.ts is not enough to fix the issue.

ofrobots commented 5 years ago

@lostpebble

Consumers of libraries should not be forced in to any kind of tsconfig.json setting - as these modules don't exist in a vacuum.

I agree. That's why there is no setting we can have that makes all users happy. This ultimately is a problem in how TypeScript and esModuleInterop and that the setting leaks from libraries to users.

DominicKramer commented 5 years ago

Consider a repo with the following structure:

app
+ src
   index.ts
   dependency.d.ts
tsconfig.json
package.json

where app/src/index.ts contains:

import * as Dependency from './dependency';
class MyClass extends Dependency {}

and app/src/dependency.d.ts contains:

export = dependency;

interface DependencyConstructor {
  new(): dependency.Dependency;
}
declare var dependency: DependencyConstructor;
declare namespace dependency {
  interface Dependency {
  }
}

Then running tsc -p . with the following tsconfig.json:

{
  "compilerOptions": {
    "rootDir": ".",
    "outDir": "build",
    "lib": ["es2016"],
    "module": "commonjs",
    "esModuleInterop": false
  },
  "include": [
    "src/*.ts"
  ]
}

compiles correctly. However, with tsconfig.json:

{
  "compilerOptions": {
    "rootDir": ".",
    "outDir": "build",
    "lib": ["es2016"],
    "module": "commonjs",
    "esModuleInterop": true
  },
  "include": [
    "src/*.ts"
  ]
}

Then tsc -p . fails with the following error:

src/index.ts:4:23 - error TS2507: Type '{ default: DependencyConstructor; }' is not a constructor function type.

4 class MyClass extends Dependency {}
                        ~~~~~~~~~~

Changing app/src/dependency.d.ts to:


export = dependency;

interface DependencyConstructor {
  new(): dependency.Dependency;
  default: DependencyConstructor; // <- Note: default set here
}
declare var dependency: DependencyConstructor;
declare namespace dependency {
  interface Dependency {
  }
}

Allows compilation to succeed with either esModuleInterop set to true or false.

This setup is a minimal repro that behaves the way @types/duplexify defines its types. Thus, it looks like to address this issue, a change to @types/duplexify is needed.

DominicKramer commented 5 years ago

I'm closing this issue since it appears to be a issue with duplexify. I have a PR open at https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33920 to update duplexify.