Fix five files with missing type information

[JamesSkemp]

Fixes part of #247.

https://github.com/JamesSkemp/rpg-dice-roller-test is a simple repo that verifies the original bug and can be used to verify this partial fix.

There are seven files that have issues and this fixes five of those files. These all relate to the JS referencing types in documentation that are not referenced by the code itself.

Importing these into the files (and having ESLint ignore that they aren't used by the code) correctly adds them to the TypeScript definitions file.

To verify this fix:

1. Clone https://github.com/JamesSkemp/rpg-dice-roller-test and run npm install and npm run build. index.js will be created, but TypeScript will report 17 errors in 7 files.
2. In this branch, run npm run build:declaration to generate the TypeScript declaration files.
3. Replace the rpg-dice-roller-test/node_modules/@dice-roller/rpg-dice-roller/types directory with the contents of rpg-dice-roller/types
4. Run npm run build from the rpg-dice-roller-test repo. There will be 6 errors in 2 files.

[GreenImp]

Thanks @JamesSkemp , looks good! What's the reason for it not being done on those other two files?

[JamesSkemp]

:)

It's mentioned in #247, but if I add those types in, ESLint detects a dependency cycle. I've been living in mostly TS for a while, so from what I can gather, if I add a reference to StandardDice in one/both of those files, for example, StandardDice also has a dependency back to both of those files. And then we have a loop ...

It's a 'soft' dependency, since only the comments actually depend upon it, but I'm rusty enough with new-school JS that I don't trust it won't break the JS build in some case.

[GreenImp]

I've been trying to resolve those two files by changing how the modules are imported, on this branch: https://github.com/dice-roller/rpg-dice-roller/tree/bugfix/typescript-import-fixes.

Some things can be fixed by importing the module directly, rather than via the index.js files.

e.g.:

import { CompareOperatorError, RequiredArgumentError } from './exceptions/index.js';

Becomes:

import CompareOperatorError from './exceptions/CompareOperatorError.js';
import RequiredArgumentError from './exceptions/RequiredArgumentError.js';

But I still get stuck with a few dependency cycles. I haven't found an ideal way around it yet.
I was hoping there would be a good way of importing them just within the DocBlock, so it works for Typescript, but doesn't throw a dependency cycle within the code, but I can't see a way of doing that.

[JamesSkemp]

Sorry, the notification about this must have been buried in my email somewhere.

I'll try to pull down your branch this weekend and see if a fresh set of eyes on the issue gets me anywhere.

[JamesSkemp]

So I thought maybe something could be changed in the declaration.tsconfig.json that would allow explicit importing or mapping during the declaration creating, but neither https://www.typescriptlang.org/tsconfig#type-include nor https://www.typescriptlang.org/tsconfig#types appear to be working.

(insert possible Eureka moment on some better search terms and 30 minutes of searching and testing)

/**
 * @typedef {import("random-js").Engine} Engine
 */

If you plop that near the top of src\utilities\NumberGenerator.js, immediately after the import, regenerate definitions, and test those out in a TypeScript project (I've updated https://github.com/JamesSkemp/rpg-dice-roller-test for 5.1.1 so that could be used for the base project to replace node_modules types) that clears up the Engine related issues in that file.

[JamesSkemp]

And it only fixes that one file ...

Adding the following (and a couple variations) in FudgeDice.js doesn't resolve the issue there.

/**
 * @typedef {import("../modifiers/Modifier").Modifier} Modifier
 */

Might just be an issue with the declaration.

[JamesSkemp]

Before I forget, there was one source that suggested creating a js file specific to type definitions that would import and export the necessary typedefs.

Perhaps if a types.js file existed that imported and exported types, then typedefs could import that file and it would resolve it?

[GreenImp]

And it only fixes that one file ...

Adding the following (and a couple variations) in FudgeDice.js doesn't resolve the issue there.

/**
 * @typedef {import("../modifiers/Modifier").Modifier} Modifier
 */

Might just be an issue with the declaration.

This is exactly the kind of thing I was hoping for - so we can tell Typescript what the definition is, without creating the circular dependency.

Where did you find info on using the import syntax in the typedef? All I can find in the docs is this, which doesn't provide any example of that syntax: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#typedef-callback-and-param

---

Edit: Just a thought, it could be that the Modifier file doesn't actually return a Modifier property. It just returns a default:

export default Modifier;

So it might work with something like:

/**
 * @typedef {import("../modifiers/Modifier")} Modifier
 * Or:
 * @typedef {import("../modifiers/index.js").Modifier} Modifier
 */

[GreenImp]

Hmm,. I'm not sure that it works correctly either. I've just checked your Engine one:

/**
 * @typedef {import("random-js").Engine} Engine
 */

And it produces the following:

export type Engine = import("random-js").Engine;

...

export const engines: {
    ...
    browserCrypto: Engine;
    nodeCrypto: Engine;
    MersenneTwister19937: MersenneTwister19937;
    nativeMath: Engine;
};

...

declare class NumberGenerator {
    ...

    constructor(engine?: import("random-js").Engine | {
        next(): number;
    } | undefined);

    ...
}

It's changed the function arguments from Engine to import("random-js").Engine, but other uses of Engine look correct 🤷

[JamesSkemp]

It was a mix of these, plus potentially some other pages that I closed/found earlier in the evening:

• https://stackoverflow.com/a/73551192/11912
• https://stackoverflow.com/a/60572888/11912
• https://stackoverflow.com/a/72223802/11912

Here's the 'have a different file just for types' idea: https://stackoverflow.com/a/73232942/11912

Sorry for the delay, had these all up on my Windows machine.

[GreenImp]

Thanks for those links! From that I've also found the it actually documented on the Typescript site: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#import-types (Same page I was looking at before, but further down).

[GreenImp]

I've realised that using the @typedef method seems to cause the documentation generation (npm run build:docs) to fail, for some reason.
I've not managed to get an error response from it, it just says that the file was "Excluded", but I think it's because JSDoc doesn't understand the import syntax of the type definition: https://jsdoc.app/tags-typedef.html

If I take out the @typedef it works okay again.

[GreenImp]

I thought I'd take a look at this from a different angle. I've been playing around with seeing what actually happens with the circular dependencies.

Instead of adding the @typedef import bits, I've just told ESLint to ignore circular dependencies on the relevant lines:

// eslint-disable-next-line import/no-cycle

And then ESLint is happy. And the tests all seem to run okay, and I can successfully build the compiled versions. So this may be the way to go.