TypeScript definitions - Missing argument in DiceRoller constructor and Map type

[JamesSkemp]

Describe the bug With TypeScript 3.9.6 and version 4.3.0 (and 4.2.0) TypeScript is issuing the following complaints:

DiceRoller

Expected 1 arguments, but got 0.
  > 458 |   const rpgDiceRoller = new DiceRoller();

Map

ERROR in /path/to/project/node_modules/rpg-dice-roller/types/dice/PercentileDice.d.ts(9,74):
9:74 Generic type 'Map<K, V>' requires 2 type argument(s).
     7 |      * @param {boolean=} sidesAsNumber whether to show the sides as a number or the percent symbol
     8 |      */
  >  9 |     constructor(notation: string, qty?: number | undefined, modifiers?: (Map | {} | Map[] | null) | undefined, sidesAsNumber?: boolean | undefined);
       |                                                                          ^
    10 |     sidesAsNumber: boolean;
    11 | }
    12 | import StandardDice from "./StandardDice";
ERROR in /path/to/project/node_modules/rpg-dice-roller/types/dice/PercentileDice.d.ts(9,85):
9:85 Generic type 'Map<K, V>' requires 2 type argument(s).
     7 |      * @param {boolean=} sidesAsNumber whether to show the sides as a number or the percent symbol
     8 |      */
  >  9 |     constructor(notation: string, qty?: number | undefined, modifiers?: (Map | {} | Map[] | null) | undefined, sidesAsNumber?: boolean | undefined);
       |                                                                                     ^
    10 |     sidesAsNumber: boolean;
    11 | }
    12 | import StandardDice from "./StandardDice";
ERROR in /path/to/project/node_modules/rpg-dice-roller/types/dice/StandardDice.d.ts(11,89):
11:89 Generic type 'Map<K, V>' requires 2 type argument(s).
     9 |      * @param {?number=} max The maximum possible roll value (Defaults to the value of sides)
    10 |      */
  > 11 |     constructor(notation: string, sides: number, qty?: number | undefined, modifiers?: (Map | {} | Map[] | null) | undefined, min?: (number | null) | undefined, max?: (number | null) | undefined);
       |                                                                                         ^
    12 |     /**
    13 |      * Sets the modifiers that affect this roll
    14 |      *
ERROR in /path/to/project/node_modules/rpg-dice-roller/types/dice/StandardDice.d.ts(11,100):
11:100 Generic type 'Map<K, V>' requires 2 type argument(s).
     9 |      * @param {?number=} max The maximum possible roll value (Defaults to the value of sides)
    10 |      */
  > 11 |     constructor(notation: string, sides: number, qty?: number | undefined, modifiers?: (Map | {} | Map[] | null) | undefined, min?: (number | null) | undefined, max?: (number | null) | undefined);
       |                                                                                                    ^
    12 |     /**
    13 |      * Sets the modifiers that affect this roll
    14 |      *

To Reproduce Steps to reproduce the behavior:

(Using Visual Studio Code for built-in TypeScript support.)

1. Setup a new project, with npm install typescript eslint rpg-dice-roller
2. Create a new TypeScript file (index.ts)
3. Import library and create a new DiceRoller with no arguments.

import { DiceRoller } from 'rpg-dice-roller';
let roller = new DiceRoller();

Above reports an issue with DiceRoller() constructor missing an argument.

I'm having more trouble creating a small reproducible test case for the Map-related issues (it's stopping my build in a VueJS project, but not in a project that doesn't use VueJS), but opening /path/to/project/node_modules/rpg-dice-roller/types/dice/StandardDice.d.ts is showing the error in VS Code.

Expected behavior DiceRoller(): data just needs to be marked as optional, to match documentation.

I'm not current enough on JS to understand what's up with Map (I've been reading up on it as I can), but ideally these would be output so that TypeScript doesn't complain about them.

Screenshots / Code snippets Screenshot for Map issue, in case it helps:

Environment (please complete the following information):

• Environment type: NodeJs, TypeScript 3.9.6, VS Code 1.47.2 (Mac)
• Version 4.2.0 and 4.3.0

Additional context

Optional constructor param should be easy enough.

In JSDoc, looks like it may be changing from @param data to @param [data={}] - Optional object with a log property containing an array of DiceRoll objects.

But I don't know enough about Map at this point.

[GreenImp]

I've gone through all the doc blocks for every file and updated them, and also making sure they match the JSDoc style, instead of the Google style (Which is mainly how optional parameters are defined).

For maps, you need to define the key and value types as well:

// incorrect
* @param {Map} arg1

// correct
* @param {Map<string, number>} arg2

I've had a quick test and it looks like it's working now. I'm using PHPStorm, but I'm sure the Typescript detection is much the same. I've tried creating both DiceRoll and DiceRoller objects; changing the number generator; and outputting the result.

I've got it set up on branch bugfix/166-typeScript-definitions-missing-argument-in-DiceR. As my Typescript knowledge is lacking, I'd really appreciate it if you could give it a whirl and see if it works, before I merge it.

You can install a particular git branch with:

npm install git://github.com/GreenImp/rpg-dice-roller.git#bugfix/166-typeScript-definitions-missing-argument-in-DiceR --save

If you're happy that it's working, I'll get it merged in.

[JamesSkemp]

Hmm. Installing that branch isn't working. This is all it's installing for me in node_modules:

However, I'll pull down that branch, build it, and copy it over to node_modules; that should do the trick. :)

[JamesSkemp]

Checking out the branch, installing, building, building declarations, and then manually copying over the types gets things closer.

It's still complaining about data being required when creating a DiceRoller, but is now complaining about readonlys. Map is resolved though. :)

ERROR in /path/to/project/node_modules/rpg-dice-roller/types/results/RollResult.d.ts(81,5):
81:5 'readonly' modifier can only appear on a property declaration or index signature.
    79 |      * @returns {number}
    80 |      */
  > 81 |     readonly get initialValue(): number;
       |     ^
    82 |     /**
    83 |      * Returns the flags for the modifiers that affect the roll
    84 |      *

 error  in /path/to/project/node_modules/rpg-dice-roller/types/results/RollResults.d.ts

ERROR in /path/to/project/node_modules/rpg-dice-roller/types/results/RollResults.d.ts(35,5):
35:5 'readonly' modifier can only appear on a property declaration or index signature.
    33 |      * @returns {number}
    34 |      */
  > 35 |     readonly get length(): number;
       |     ^
    36 |     /**
    37 |      * The total value of the rolls, taking in to consideration modifiers
    38 |      *

 error  in /path/to/project/node_modules/rpg-dice-roller/types/results/RollResults.d.ts

ERROR in /path/to/project/node_modules/rpg-dice-roller/types/results/RollResults.d.ts(43,5):
43:5 'readonly' modifier can only appear on a property declaration or index signature.
    41 |      * @returns {number}
    42 |      */
  > 43 |     readonly get value(): number;
       |     ^
    44 |     /**
    45 |      * Adds a single roll to the list
    46 |      *

Again, I'm behind on newer JS practices, but looking at the JS it does seem @readonlys on those may have been in error?

[GreenImp]

Strange, I thought I'd removed those. I'll double check now. It was quite late last night that I was working on it.

[GreenImp]

Ah yeah, sorry, I forgot you need to build the types locally. How you're doing it there is absolutely correct.

I've updated the doc blocks:

• Removed @readonly - except on diceUtils and exportFormats
• Make DiceRoller constructor's data argument optional (Not sure how this was missed)
• Fix declaration of diceUtils - it was missing definitions of it's properties
• Fix declaration of NumberGenerator.engines - it was missing info for the engines themselves

Hopefully that should work, if you pull down that branch again.

[JamesSkemp]

That did the trick!

Awesome. :)

[GreenImp]

Fantastic. I'll merge it in and get a release out.