Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(7.x.x) - Evaluation of types.d according to jsDoc #189

Closed
swingingtom opened this issue Apr 18, 2022 · 12 comments
Closed

(7.x.x) - Evaluation of types.d according to jsDoc #189

swingingtom opened this issue Apr 18, 2022 · 12 comments
Labels
7.x.x Task or requests for 7.x.x Looking 4 Volunteer

Comments

@swingingtom
Copy link
Collaborator

#188 Refactor introduced a tsconfig.json that scan js files to automatically build types.

This could be a nice way to stop being late on typescript on each update but an evaluation of constraints, do and don'ts should be performed prior.

@swingingtom swingingtom added 7.x.x Task or requests for 7.x.x Looking 4 Volunteer labels Apr 18, 2022
@swingingtom
Copy link
Collaborator Author

We are looking for a typescript user for this task

@JoeCoppola-HIA
Copy link
Contributor

Is the idea here that you want https://github.com/felixmariotto/three-mesh-ui/blob/e5cee5b14a2d6bed9c62b21ad51c1309c520b6e0/src/types.d.ts evaluated to see if the auto generated types are accurate? And do we want all types to be evaluated or just font related ones this time around?

@swingingtom
Copy link
Collaborator Author

Hey @JoeCoppola-HIA , I've manually written that part of the types definition, and commented out as I wasn't sure it would be better than the simpler placed below

export declare namespace FontLibrary {
	export function addFontFamily(fontFamilyName:string);
	export function getFontFamily(fontFamilyName:string);
	export function load(fontFamily:any, ...args:any[]):Promise<unknown>;
	export function setMissingCharacterHandler(fontVariant:any, character:string):string|null;
	export function missingCharacter(handler:(fontVariant:any, character:string) => string|null);
}

Manually writing or updating it, isn't very exciting at all. And some names are gonna be refactored very soon.

It would be good to keep types definition as accurate as possible, even if it is not fully complete / useable.

The evaluation that could be done here, is : from the current output of $ npx tsc + tsconfig.json + current JsDoc coverage

  • Is the output useable as is ? What need to be done (jsdoc ideally) to make it useable ?
  • What is the accuracy of the output result when jsdoc is okay
  • Is the output of not jsdocumented total garbage or is it still okay?
  • What are do's. What are dont's (Im looking at you mix)

Having automation and guidelines could help to prevent types definition to be completely outdated, without the need to manually edit those.

@swingingtom
Copy link
Collaborator Author

At the moment, that evaluation should be done on a branch from 7.x.x-feature/font-class-font-library

@JoeCoppola-HIA
Copy link
Contributor

Thank you for the explanation. As we are heavily invested into the typescript side and are using several of the more advanced APIs, I think we can help evaluate the current state of the output from tsc.

@JoeCoppola-HIA
Copy link
Contributor

Generated Output:

Attached is the image of generated type files from running npx tsc minus the main *.d.ts file.

image

The generated three-mesh-ui.d.ts file reads as followed:

import Block from "./components/Block.js";
import Text from "./components/Text.js";
import InlineBlock from "./components/InlineBlock.js";
import Keyboard from "./components/Keyboard.js";
import FontLibrary from "./components/core/FontLibrary.js";
export function update(): void;
import * as TextAlign from "./utils/inline-layout/TextAlign";
import * as Whitespace from "./utils/inline-layout/Whitespace";
import * as JustifyContent from "./utils/block-layout/JustifyContent";
import * as AlignItems from "./utils/block-layout/AlignItems";
import * as ContentDirection from "./utils/block-layout/ContentDirection";
declare namespace ThreeMeshUI {
    export { Block };
    export { Text };
    export { InlineBlock };
    export { Keyboard };
    export { FontLibrary };
    export { update };
    export { TextAlign };
    export { Whitespace };
    export { JustifyContent };
    export { AlignItems };
    export { ContentDirection };
}
export { Block, Text, InlineBlock, Keyboard, FontLibrary, TextAlign, Whitespace, JustifyContent, AlignItems, ContentDirection };

Current Evaluation:

This is a useful start, but if I understand the output correctly at the moment, as well as the errors being spit out by VS Code here, it isn't usable in its current state just yet. A majority of the problems seem to stem from converting jsdoc param comments into types in the *.d.ts files but failing to add the appropriate imports needed to find the type.

image

Take the following example:

TextAlign.js has a jsdoc comment for the textAlign function as such:

/**
 *
 * @param {Array.<Array.<InlineCharacter>>} lines
 * @param ALIGNMENT
 * @param INNER_WIDTH
 */
export function textAlign( lines, ALIGNMENT, INNER_WIDTH ) {
...

It defines lines as an Array.<Array.<InlineCharacter>> but the js file does not import InlineCharacter as it doesn't directly use the class. The resulting TextAlign.d.ts file reads as such

/**
 *
 * @param {Array.<Array.<InlineCharacter>>} lines
 * @param ALIGNMENT
 * @param INNER_WIDTH
 */
export function textAlign(lines: Array<Array<InlineCharacter>>, ALIGNMENT: any, INNER_WIDTH: any): void;
...

And TS compilers complain that InlineCharacter can not be resolved here.

Potential Solution:

Simply adding the import import InlineCharacter from "../../font/InlineCharacter"; in TextAlign.js will result in TextAlign.d.ts importing the appropriate file and resolving this issue.

I plan to read a little more up on how tsc parses jsdoc comments to see if there is a way to have it handle these imports for us, or if we have to simply do what I stated above.

@JoeCoppola-HIA
Copy link
Contributor

JoeCoppola-HIA commented Apr 18, 2022

Another potential concern that I haven't been able to evaluate just yet is how the mix.withBase implementation may prevent classes like Block from exporting with their "parent class" properties such as properties inherited from MeshUIComponent.js.

Right now any class that uses mix.withBase simply declares a base class as "any" and extends that in the type files, like this example from Block.d.ts:

declare const Block_base: any;
/**

Job:
- Update a Block component
- Calls BoxComponent's API to position its children box components
- Calls InlineManager's API to position its children inline components
- Call creation and update functions of its background planes

 */
export default class Block extends Block_base {
...

@swingingtom
Copy link
Collaborator Author

I've just discover import jsdoc directive

 /**
  * @param { import('express').Request } req
  **/ 

Just wanted to let you know

@JoeCoppola-HIA
Copy link
Contributor

Hey @swingingtom,

I noticed this directive as well, and was reading microsoft/TypeScript#22160 on the general thoughts of this. This seems like it will solve the issue as well, but seems like it could result in a lot of repetitive comments in some source files.

I am fine with either this solution or the one I suggested. Just let me know what you think. If you are ok with this, I would like to clean up these errors I found and PR the fixes back to your branch. Thanks.

@swingingtom
Copy link
Collaborator Author

It might be a bit inconsistent, but perhaps we should use that only when type is not directly referenced in the file? What do you think?

And what about simply merging all definitions in one single file such as it is currently done https://github.com/felixmariotto/three-mesh-ui/blob/master/src/types.d.ts ?
Would that fix missing import? That's sound like a wacky hack fix ...

@swingingtom
Copy link
Collaborator Author

Just let me know what you think. If you are ok with this, I would like to clean up these errors I found and PR the fixes back to your branch.

If you think it would reduce the current types gap, go for it. Thanks

@JoeCoppola-HIA
Copy link
Contributor

I'm not sure I'm a big fan of a single types file personally, mainly because the compiler does a nice job of exporting the type files in the original file structure of the source which could better inform implementers of Three Mesh UI original design intent.

I tried outputting the types to a single file just now and it still has the same import issues. This occurs because while it is still in the same file, the modules declared keep their imports local to the module, and not to the global file.

microsoft/TypeScript#22160 (comment)
This person had the same solution that I came up with and it seems to work well for them.

I am going to take a first stab at updating this to resolve the errors, which should then allow me to further evaluate the types. One thing I plan on adding that may be up for discussion is adding some additional declarations in src/three-mesh-ui.js. I will be sure to explain why I think they should be there in the PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x.x Task or requests for 7.x.x Looking 4 Volunteer
Projects
None yet
Development

No branches or pull requests

2 participants