melonjs / melonJS

a fresh, modern & lightweight HTML5 game engine
https://melonjs.org
MIT License
5.88k stars 646 forks source link

TypeScript defenition for loader.preload incorrect #1185

Closed edmundg closed 1 year ago

edmundg commented 1 year ago

Describe the bug

When using TypeScript with the solution there is an issue reported with the preload function defenition.

To Reproduce

  1. Clone the standard boiler plate code and follow the instructions.
  2. Rename all .js files to .ts
  3. Call npm install typescript
  4. Add a TS config similar to:

    {
    "compilerOptions": {
    "target": "ES2020",
    "useDefineForClassFields": true,
    "module": "ESNext",
    "lib": ["ES2020", "DOM", "DOM.Iterable"],
    "skipLibCheck": true,
    
    /* Bundler mode */
    "moduleResolution": "bundler",
    "allowImportingTsExtensions": true,
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    
    /* Linting */
    "strict": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noFallthroughCasesInSwitch": true
    },
    "include": ["src"]
    }

Expected behavior When providing a manifest.ts file as below then no error should be reported within the preloader in main.ts

const DataManifest = [
    {

    }
];

export default DataManifest;

Error returned: Argument of type '{ name: string; type: string; src: string; }[]' is not assignable to parameter of type '{ name: string; type: string; src: string; stream?: boolean | undefined; }'. Type '{ name: string; type: string; src: string; }[]' is missing the following properties from type '{ name: string; type: string; src: string; stream?: boolean | undefined; }': name, type, srcts(2345)

Screenshots

image

Live Example

Public repo with issue in Github https://github.com/edmundg/tktsengine

Device (please complete the following information):

Workaround Edit the manifest.ts file setting the DataManifest type to any

const DataManifest: any = [
    {

    }
];

export default DataManifest;
obiot commented 1 year ago

yes so, the issue seems to be that tsc does not recognized/parse the below as an array of object, even though it is declared as of one (using {object[]} : https://github.com/melonjs/melonJS/blob/master/src/loader/loader.js#L108-L114

as you can see below the corresponding definition just include the object definition and lacks the array part : https://github.com/melonjs/melonJS/blob/master/dist/types/loader/loader.d.ts#L46-L51

I just played around a bit and changed the declaration to the below :

 * @param {Array.<object>} res
 * @param {string} res.name - internal name of the resource
 * @param {string} res.type  - "audio", binary", "image", "json","js", "tmx", "tsx", "fontface"
 * @param {string} res.src  - path and/or file name of the resource (for audio assets only the path is required)
 * @param {boolean} [res.stream] - Set to true to force HTML5 Audio, which allows not to wait for large file to be downloaded before playing.
 * @param {Function} [onloadcb=loader.onload] - function to be called when all resources are loaded
 * @param {boolean} [switchToLoadState=true] - automatically switch to the loading screen

and then the TS typing is more correct, but then we loose the definition of the object itself

export function preload(res: Array<object>, onloadcb?: Function | undefined, switchToLoadState?: boolean | undefined): void;
obiot commented 1 year ago

oh well I just realized that our documentation is also missing a bit of declaration, so I guess maybe we used/declared things the wrong way since the beginning here : https://melonjs.github.io/melonJS/docs/melonjs/loader.html#preload

I need some searching on how to fix this the proper way now :)

obiot commented 1 year ago

done, this works :

/**
 * @typedef {object} loader.Asset
 * @property {string} Asset.name - internal name of the resource
 * @property {string} Asset.type  - "audio", binary", "image", "json","js", "tmx", "tsx", "fontface"
 * @property {string} Asset.src  - path and/or file name of the resource (for audio assets only the path is required)
 * @property {boolean} [Asset.stream] - Set to true to force HTML5 Audio, which allows not to wait for large file to be downloaded before playing.
 * @memberof loader
 */

/**
 * set all the specified game assets to be preloaded.
 * @name preload
 * @memberof loader
 * @public
 * @param {loader.Asset[]} assets - list of assets to load
 export function preload(assets, onloadcb, switchToLoadState = true) {
    ...
 }
 */

I just added a new Asset typedef, and it properly works everywhere, and generate the below in TS :

/**
 * an asset definition to be used with the loader
 * @typedef {object} loader.Asset
 * @property {string} name - name of the asset
 * @property {string} type  - the type of the asset : "audio", binary", "image", "json","js", "tmx", "tsx", "fontface"
 * @property {string} src  - path and/or file name of the resource (for audio assets only the path is required)
 * @property {boolean} [stream] - Set to true to force HTML5 Audio, which allows not to wait for large file to be downloaded before playing.
 * @see loader.preload
 * @see loader.load
 */
/**
 * set all the specified game assets to be preloaded.
 * @name preload
 * @memberof loader
 * @public
 * @param {loader.Asset[]} assets - list of assets to load
 * @param {Function} [onloadcb=loader.onload] - function to be called when all resources are loaded
 * @param {boolean} [switchToLoadState=true] - automatically switch to the loading screen
 */
export function preload(assets: loader.Asset[], onloadcb?: Function | undefined, switchToLoadState?: boolean | undefined): void;

I will clean up everything look for other <any> type declaration in the loader and publish a new version probably today.

obiot commented 1 year ago

Hi @edmundg, I just published a 15.1.4 version on NPM that should work now.

I haven't updated the boilerplate yet, I'll wait for you to confirm first.

Note that we still have a bunch of any type declaration across the framework, but those should not cause this kind of issue. Anyway, if you find more causing problem, let us know !

edmundg commented 1 year ago

Thanks @obiot I can confirm the issue is now resolved, I have updated to the latest version. I recreated the issue, updated and confirm the bug has been fixed.

I will leave the bug open for now in case you want to update the boiler plate first.

obiot commented 1 year ago

awesome, thanks for confirming, and closing this one then :)