-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add TypeScript typings #54
base: main
Are you sure you want to change the base?
Conversation
take it or leave it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I would love this! I'm not a core member, but maybe this review will help push things along.
* _string_) to remove all `any` usages. | ||
*/ | ||
|
||
declare type HarfbuzzPointer = string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be number
ax: number; | ||
ay: number; | ||
dx: number; | ||
dy: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add flags: number;
(recently added, not yet released)?
ptr: HarfbuzzPointer; | ||
addText: (text: string) => void; | ||
guessSegmentProperties: () => | ||
any; // any = return of exports.hb_buffer_guess_segment_properties(ptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C++ hb_buffer_guess_segment_properties
returns void
, this can just be void
too
setLanguage: (language: string) => void; | ||
setScript: (script: string) => void; | ||
setClusterLevel: (level: number) => void; | ||
json: (font?: HarfbuzzFont) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't have an argument, should just be ()
(font: HarfbuzzFont, buffer: HarfbuzzBuffer, features: string, | ||
stop_at: number, stop_phase: number) => HarfbuzzJson; | ||
} | ||
declare function hbjs(instance: HarfbuzzModule): HarfbuzzJs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most people will be doing require('harfbuzzjs')
, but this doesn't appear to make that typed. It looks like it's for require('./hbjs')
. Does there need to be another file index.d.ts
?
I tried a few things but couldn't get hbjs.d.ts
to work (for example correct hover types in VS Code). If it helps, a while ago I wrote my own types via a root-level types.d.ts
that looks like this:
declare namespace HarfBuzzJsInit {
type HbFace = {};
...
type Harfbuzz = {
createBlob
...
};
}
// declaring a const with the same name as the namespace above
// lets people do `import {HbFace} from 'harfbuzzjs'
declare const HarfbuzzJsInit: Promise<HarfBuzzJsInit.HarfBuzz>;
export = HarfBuzzJsInit;
cc @ebraminio |
I think this one can be closed since it has problems and the author said "take it or leave it" 🙂 |
take it or leave it