arcanis / clipanion

Type-safe CLI library / framework with no runtime dependencies
https://mael.dev/clipanion/
1.12k stars 66 forks source link

bug: ts issues in d.ts file #25

Closed regevbr closed 4 years ago

regevbr commented 4 years ago

Hi @arcanis My PR in https://github.com/snyk/nodejs-lockfile-parser/pull/57 is using @yarnpkg/core which in turn depends on this library (2.4.1) but the d.ts file of the published npm files causes compilation issues.

node_modules/clipanion/lib/index.d.ts:2:23 - error TS2688: Cannot find type definition file for 'mocha'.

2 /// <reference types="mocha" />
                        ~~~~~

node_modules/clipanion/lib/index.d.ts:449:32 - error TS2339: Property 'binaryName$0' does not exist on type '{ binaryLabel?: string | undefined; binaryName?: string | undefined; binaryVersion?: string | undefined; enableColors?: boolean | undefined; }'.

449     constructor({ binaryLabel, binaryName$0, binaryVersion, enableColors }?: {

I will try to create a quick PR to fix it

regevbr commented 4 years ago

The 2nd issue happens due to naming collision in the unified deceleration file and can be easily solved by giving the spread parameter a different name.

Spent a couple of hours investigating the 1st issue. I managed to understand that because @types/mocha declares a namespace for NodeJs -

declare namespace NodeJS {
    // Forward declaration for `NodeJS.EventEmitter` from node.d.ts.
    // Required by Mocha.Runnable, Mocha.Runner, and Mocha.Suite.
    // NOTE: Mocha *must not* have a direct dependency on @types/node.
    // tslint:disable-next-line no-empty-interface
    interface EventEmitter { }

    // Augments NodeJS's `global` object when node.d.ts is loaded
    // tslint:disable-next-line no-empty-interface
    interface Global extends Mocha.MochaGlobals { }
}

It causes typescript to assume that mocha is a referenced type. The strange thing is that if you run plain tsc command, the node referenced type is emitted but not the mocha one...

The issue first appears in the rollup-plugin-typescript in the typeReferenceCollector function (line 6465). I will keep investigating on how to resolve this.

@arcanis this can be easily solved if we separate the test dependencies, maybe by putting it in a different sub package.When I remove the @types/mocha as a dependency the issue got resolved...

p.s I tried other rollup typescript plugins but hey all failed to run smoothly to begin with :-(

regevbr commented 4 years ago

So it seems that solution was easy - declare explicitly the types in tsconfig and not put mocha there when bundling

arcanis commented 4 years ago

The 2nd issue happens due to naming collision in the unified deceleration file and can be easily solved by giving the spread parameter a different name.

What is binaryName colliding with? What makes it different from, say, binaryLabel? 🤔 Is it just a bug in TS?

regevbr commented 4 years ago

Seems to be a bug in the rollup ts plugin. I will open 2 issues there later today with reproduction repos

regevbr commented 4 years ago

The roolup ts plugin causes the issue, it thinks it collides with binaryName from Core.ts for some reason. This is not a typescript issue but the plugins way to handle conflicts while merging the deceleration files.

arcanis commented 4 years ago

Released in 2.4.2

regevbr commented 4 years ago

Thanks @arcanis !