amplitude / Amplitude-TypeScript

TypeScript Amplitude Analytics SDK
https://amplitude.github.io/Amplitude-TypeScript/
MIT License
142 stars 40 forks source link

@amplitude/analytics-browser triggers the loading of @types/node which affects the signature of setTimeout #200

Closed stof closed 2 years ago

stof commented 2 years ago

Expected Behavior

Importing @amplitude/analytics-browser should not force the loading of types defined in @types/node

Current Behavior

@amplitude/analytics-browser imports @amplitude/analytics-types, which references the node types:

node_modules/@types/node/index.d.ts
  Type library referenced via 'node' from file 'node_modules/@amplitude/analytics-types/lib/response.d.ts' with packageId '@types/node/index.d.ts@18.7.6'

The beginning of node_modules/@amplitude/analytics-types/lib/response.d.ts looks like that:

/// <reference types="node" />
import { Status } from './status';

This reference to the node types is inserted by the compiler due to using NodeJS.ErrnoException in the type declaration of SystemErrorResponse. But this causes the loading of all of @types/node, which will then define the return type of setTimeout as being a Timeout object and not a number, which is very annoying when writing a project targetting the web.

Possible Solution

Types relying on Node types should be defined in @amplitude/analytics-node rather than @amplitude/analytics-types. Or a different definition of SystemErrorResponse should be provided.

Steps to Reproduce

  1. yarn add --dev @amplitude/analytics-browser @types/node
  2. Add this code in test.ts:

    import * as amplitude from '@amplitude/analytics-browser'
    
    const a: number = setTimeout(function () {})
  3. Add this code in tsconfig.json:

    {
        "compilerOptions": {
            "noEmit": true,
            "esModuleInterop": true,
            "lib": ["dom"],
            "strict": true,
            "types": []
        },
        "files": [
            "test.ts"
        ]
    }
    
  4. Run the typescript type checker on that code (tsc -p .)
  5. See the error Type 'Timeout' is not assignable to type 'number'. reported on the a variable assignment
  6. Comment the import of Amplitude and see the error disappear.

Environment

stof commented 2 years ago

@kevinpagtakhan any idea about when this could be fixed ? It is quite annoying that importing @amplitude/analytics-browser breaks the type checking of unrelated code in our project.

kevinpagtakhan commented 2 years ago

Hey @stof! Thanks for choosing Amplitude. I opened a PR to remove the dependency on @types/node in our internal types package. I'll have some folks take a look at it and we'll get it out as soon as we can.

stof commented 2 years ago

Thanks