[opentype.js] Improve Glyph #61702

alecmev · 2022-08-12T18:00:43Z

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.
Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/opentypejs/opentype.js/blob/0de6c5482b888405fcace991a1b595f20a3a909a/src/glyph.js#L61-L99
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

The actual change is in the second commit, the first one is a Prettier pass.

typescript-bot · 2022-08-12T18:01:17Z

@alecmev Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

opentype.js — on npm, on unpkg
- owner-approval: @danmarshall

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners, DT maintainers or others

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 61702,
  "author": "alecmev",
  "headCommitOid": "fb9c1adc735f5e5a3de644b3f9cfda77b072c14f",
  "mergeBaseOid": "3f99ce278474a3f7a8bdf783dd7a2aa30849c686",
  "lastPushDate": "2022-08-12T17:57:39.000Z",
  "lastActivityDate": "2022-08-12T18:25:20.000Z",
  "mergeOfferDate": "2022-08-12T18:24:47.000Z",
  "mergeRequestDate": "2022-08-12T18:25:20.000Z",
  "mergeRequestUser": "alecmev",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "opentype.js",
      "kind": "edit",
      "files": [
        {
          "path": "types/opentype.js/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/opentype.js/opentype.js-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "danmarshall",
        "edzis",
        "peterblazejewicz"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "danmarshall",
      "date": "2022-08-12T18:24:05.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1213374453,
  "ciResult": "pass"
}

typescript-bot · 2022-08-12T18:01:19Z

🔔 @danmarshall @edzis @peterblazejewicz — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

alecmev · 2022-08-12T18:02:49Z

types/opentype.js/index.d.ts

+    yMax?: number | undefined;
+    advanceWidth?: number | undefined;
+    leftSideBearing?: number | undefined;
+    path: Path;


The getter always returns a Path, while the setter also accepts () => Path, but I think it's more practical to not include it in the type. It's not possible to type them separately right now, unfortunately: microsoft/TypeScript#43662

alecmev · 2022-08-12T18:03:14Z

types/opentype.js/index.d.ts

+    xMin?: number | undefined;
+    xMax?: number | undefined;
+    yMin?: number | undefined;
+    yMax?: number | undefined;


These aren't private.

alecmev · 2022-08-12T18:03:36Z

types/opentype.js/index.d.ts

+    yMin?: number | undefined;
+    yMax?: number | undefined;
+    advanceWidth?: number | undefined;
+    leftSideBearing?: number | undefined;


The missing property that made me create this PR.

alecmev · 2022-08-12T18:04:11Z

types/opentype.js/index.d.ts

+    name: string | null;
+    unicode?: number | undefined;


The code has no guards against these being null / undefined.

alecmev · 2022-08-12T18:05:07Z

types/opentype.js/index.d.ts


+    private bindConstructorValues(options: GlyphOptions): void;


There's no point in calling this outside of the constructor. The library itself never uses it either.

there as no point of addition 'private', just remove from declaration, so user cannot access this (unless doing module augmentation)

Tried to google this when I was creating the PR, but couldn't find anything conclusive. My intention was to document this for future contributors, so that they don't add this back by mistake. Or is a comment preferred?

easy, improve in future, it will come back

alecmev · 2022-08-12T18:06:12Z

types/opentype.js/index.d.ts

+    yMax?: number | undefined;
+    advanceWidth?: number | undefined;
+    leftSideBearing?: number | undefined;
+    path?: Path | (() => Path) | undefined;


In this case it can, indeed, be a callback.

danmarshall

LGTM

alecmev · 2022-08-12T18:25:20Z

Ready to merge

peterblazejewicz · 2022-08-12T19:39:51Z

types/opentype.js/opentype.js-tests.ts

+// @ts-expect-error
+Glyph.bindConstructorValues({});


just remove on next iteration (private methods)

* Prettier * Improve Glyph

alecmev added 2 commits August 12, 2022 20:21

Prettier

e63f654

Improve Glyph

fb9c1ad

alecmev requested a review from peterblazejewicz as a code owner August 12, 2022 18:00

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Aug 12, 2022

alecmev commented Aug 12, 2022

View reviewed changes

danmarshall approved these changes Aug 12, 2022

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Self Merge This PR can now be self-merged by the PR author or an owner labels Aug 12, 2022

typescript-bot moved this from Waiting for Code Reviews to Waiting for Author to Merge in New Pull Request Status Board Aug 12, 2022

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Aug 12, 2022

typescript-bot merged commit 5b22548 into DefinitelyTyped:master Aug 12, 2022

alecmev deleted the opentype-glyph branch August 12, 2022 18:26

typescript-bot removed this from Recently Merged in New Pull Request Status Board Aug 12, 2022

peterblazejewicz reviewed Aug 12, 2022

View reviewed changes

shlyren pushed a commit to shlyren/DefinitelyTyped that referenced this pull request Aug 17, 2022

🤖 Merge PR DefinitelyTyped#61702 [opentype.js] Improve Glyph by @alecmev

dd4c6e5

* Prettier * Improve Glyph

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[opentype.js] Improve Glyph #61702

[opentype.js] Improve Glyph #61702

alecmev commented Aug 12, 2022

typescript-bot commented Aug 12, 2022 •

edited

typescript-bot commented Aug 12, 2022

alecmev Aug 12, 2022

alecmev Aug 12, 2022

alecmev Aug 12, 2022

alecmev Aug 12, 2022

alecmev Aug 12, 2022

peterblazejewicz Aug 12, 2022

alecmev Aug 12, 2022

peterblazejewicz Aug 12, 2022

alecmev Aug 12, 2022

danmarshall left a comment

alecmev commented Aug 12, 2022

peterblazejewicz Aug 12, 2022

		// @ts-expect-error
		Glyph.bindConstructorValues({});

[opentype.js] Improve Glyph #61702

[opentype.js] Improve Glyph #61702

Conversation

alecmev commented Aug 12, 2022

typescript-bot commented Aug 12, 2022 • edited

1 package in this PR

Code Reviews

Status

typescript-bot commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmarshall left a comment

Choose a reason for hiding this comment

alecmev commented Aug 12, 2022

Choose a reason for hiding this comment

typescript-bot commented Aug 12, 2022 •

edited