ChiriVulpes / scryfall-sdk

A Node.js SDK for https://scryfall.com/docs/api, written in TypeScript.
MIT License
99 stars 16 forks source link

`gulp build` step emits declaration files with errors. #34

Closed Ionaru closed 3 years ago

Ionaru commented 3 years ago

Summary A number of declaration files in the scryfall-sdk package contain errors, resulting in build errors on my end.

Details When using scryfall-sdk in my typescript project, I noticed that errors would pop up when building:

node_modules/scryfall-sdk/out/api/Cards.d.ts:132:5 - error TS2411: Property 'tcgplayer' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

132     tcgplayer?: string | null;
        ~~~~~~~~~

node_modules/scryfall-sdk/out/api/Cards.d.ts:133:5 - error TS2411: Property 'cardmarket' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

133     cardmarket?: string | null;
        ~~~~~~~~~~

node_modules/scryfall-sdk/out/api/Cards.d.ts:134:5 - error TS2411: Property 'cardhoarder' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

134     cardhoarder?: string | null;
        ~~~~~~~~~~~

node_modules/scryfall-sdk/out/api/Cards.d.ts:138:5 - error TS2411: Property 'gatherer' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

138     gatherer?: string | null;
        ~~~~~~~~

node_modules/scryfall-sdk/out/api/Cards.d.ts:139:5 - error TS2411: Property 'tcgplayer_decks' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

139     tcgplayer_decks?: string | null;
        ~~~~~~~~~~~~~~~

node_modules/scryfall-sdk/out/api/Cards.d.ts:140:5 - error TS2411: Property 'edhrec' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

140     edhrec?: string | null;
        ~~~~~~

node_modules/scryfall-sdk/out/api/Cards.d.ts:141:5 - error TS2411: Property 'mtgtop8' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

141     mtgtop8?: string | null;
        ~~~~~~~

Found 7 errors.

I checked out this repo and looked at the files and the way they are created. Running gulp build finished without error, but the Cards.d.ts file it created does contain the errors I noticed in my build. image

It looks like your 'gulp build' step is somehow not giving you the feedback that the regular typescript compiler does. With a small change to your tsconfig:

{
    "compilerOptions": {
        "target": "es2015",
        "module": "commonjs",
        "moduleResolution": "node",
        "forceConsistentCasingInFileNames": true,
        "removeComments": true,
        "declaration": true,
        "strict": true,
        "strictPropertyInitialization": false,
        "noImplicitReturns": true,
        "noUnusedLocals": true,
        "noUnusedParameters": false,
        "noEmitOnError": true,
        "outDir": "./out"
    },
    "files": [
        "src/Scry.ts"
    ]
}

Specifically the noEmitOnError, outDir and files fields, I was able to run the typescript compiler directly, without gulp. Output of npx tsc:

src/api/Cards.ts:151:2 - error TS2411: Property 'tcgplayer' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

151  tcgplayer?: string | null;
     ~~~~~~~~~

src/api/Cards.ts:152:2 - error TS2411: Property 'cardmarket' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

152  cardmarket?: string | null;
     ~~~~~~~~~~

src/api/Cards.ts:153:2 - error TS2411: Property 'cardhoarder' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

153  cardhoarder?: string | null;
     ~~~~~~~~~~~

src/api/Cards.ts:158:2 - error TS2411: Property 'gatherer' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

158  gatherer?: string | null;
     ~~~~~~~~

src/api/Cards.ts:159:2 - error TS2411: Property 'tcgplayer_decks' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

159  tcgplayer_decks?: string | null;
     ~~~~~~~~~~~~~~~

src/api/Cards.ts:160:2 - error TS2411: Property 'edhrec' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

160  edhrec?: string | null;
     ~~~~~~

src/api/Cards.ts:161:2 - error TS2411: Property 'mtgtop8' of type 'string | null | undefined' is not assignable to string index type 'string | null'.

161  mtgtop8?: string | null;
     ~~~~~~~

src/api/Cards.ts:381:63 - error TS2322: Type 'string | undefined' is not assignable to type 'string | number'.
  Type 'undefined' is not assignable to type 'string | number'.

381     return this.query<Card>(["cards", setCode, collectorNumber, lang]);
                                                                    ~~~~

src/api/Cards.ts:438:36 - error TS2488: Type 'CardIdentifier[] | undefined' must have a '[Symbol.iterator]()' method that returns an iterator.

438             emitter.emitAll("not_found", ...not_found);
                                                ~~~~~~~~~

src/util/MagicQuerier.ts:62:33 - error TS2345: Argument of type 'SearchError | undefined' is not assignable to parameter of type 'SearchError'.
  Type 'undefined' is not assignable to type 'SearchError'.

62              if (result || !this.canRetry(lastError)) break;
                                             ~~~~~~~~~

Found 10 errors.

The first 7 errors are the same as the ones I got in my build, which was to be expected. The last 3 errors are very surprising to me, they seem to indicate actual problems with the build and I don't know why gulp has let those slip through.

Possible solutions on your end

Workarounds on my end

Further notes I would be happy to help develop a solution for this, but changing of replacing the build system is not something I am comfortable doing without approval of the maintainers.

Possibly related to (or regression of) #11 ?

ChiriVulpes commented 3 years ago

Thanks for the very detailed report! Tbh I'm not sure why I set this up to use gulp-typescript in the first place, most of my newer projects use gulp but use the TS compiler directly, or, for ones that are this simple, they have npm scripts that just use tsc. I'll switch to the latter for this project.

As for the rest of this, I'll investigate all of this in further detail later tonight or tomorrow. Should hopefully have this all resolved then 😅

ChiriVulpes commented 3 years ago

It looks like your errors are because my tsconfig.json didn't have strict on (because I was dumb when I set this up, I guess) so I fixed that, among other updates:

Should have the fixes released shortly, just writing up a changelog now ✨

Ionaru commented 3 years ago

I can confirm the new version has fixed the build problems I was experiencing! Thank you for the blazingly fast reply and fix! 🚀