Guidance on fixing TypeScript type-definitions

[zewa666]

• Output of node_modules/.bin/electron --version: v1.8.8
• Operating System (Platform and Version): macOS

Expected Behavior
When reading up the docs for BrowserWindow.setBounds I expected the function to take a Partial<Rectangle> as first argument

Actual behavior
but instead the generated type definitions are:

/**
  * Resizes and moves the window to the supplied bounds
  */
setBounds(bounds: Rectangle, animate?: boolean): void;

Now what I'd be really looking for is some minor instruction on how to contribute such fixes by myself. Looking through the package.json of electron I noticed the use of a custom dependency to generate the definitions out of a previously generated api.json from this module. Now the later kinda sounds like the fix needs to be done inside the documentation. So after digging around the docs for the BrowserWindow show this content to define the method:

#### `win.setBounds(bounds[, animate])`

* `bounds` [Rectangle](structures/rectangle.md)
* `animate` Boolean (optional) _macOS_

Am I on the right track with my assumptions? If so how would I mark the Rectangle of the bounds property to be Partial. Are there any docs to see all the available constructs to be used as annotation or which features of TS are supported (think of Unions, Conditional Types etc.)

[welcome]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing guidelines.

[zewa666]

Whats even more funny, providing only x and y and omitting width and height leads to an exception. So clearly something is wrong with the docs or behavior of this method

[BinaryMuse]

Whats even more funny, providing only x and y and omitting width and height leads to an exception. So clearly something is wrong with the docs or behavior of this method

What version of Electron are you trying this with? The ability to use Partial<Rectangle> instead of a whole Rectangle is new in 4.0 (#15699).

Am I on the right track with my assumptions?

Yep, you're on track. The npm script create-typescript-definitions runs

npm run create-api-json && electron-typescript-definitions --in=electron-api.json --out=electron.d.ts

where create-api-json is

electron-docs-linter docs --outfile=electron-api.json

I don't think, as of yet, we've run into a situation like this where we've wanted to use a union or generic or similar in the definitions (although that could be due only to oversight). The fact that the docs are in JS and we generate TS definitions limits us unless we change something I think, but any changes you'd want to make to anything other than the docs would likely live in the electron-typescript-definitions module.

I'm open to thoughts from you or others on the best way to tackle this!

[MarshallOfSound]

If we were writing the definitions in plain typescript you are correct @zewa666 we would use Partial<Rectangle> however we aren't 😆for many long and annoying reasons that are much wider spreading that this issue.

The way we would do this with our current auto-generation set up is either declare those properties as optional in markdown in a new PartialRectangle structure. E.g.

#### `win.setBounds(bounds[, animate])`

* `bounds` [PartialRectangle](structures/partial-rectangle.md)
* `animate` Boolean (optional) _macOS_

or the better (IMO) way to do it would be to introduce a new (partial) tag similar to our (optional) tag that the linter and typescript generator would read to generate a Partial<T> instead of just T.

All this is a bit interesting because I'm currently rewriting our docs parsing logic to be lossless (electron-docs-linter is actually lossy at the moment, random bits of paragraphs code blocks and such get lost during the parsing process). Happy to push that work up somewhere if you want to play with it but it wouldn't be a good idea IMO to implement this in electron-docs-linter when I'm about to blow it out of the water 😆

[MarshallOfSound]

As a side note (untested) our current typescript generator is literally just based on strings, you might just be able to do this and it would work.

#### `win.setBounds(bounds[, animate])`

* `bounds` [Partial<Rectangle>](structures/rectangle.md)
* `animate` Boolean (optional) _macOS_

[zewa666]

@BinaryMuse ok that clarifies this issue. I'm on 1.8.8 so all good.

@MarshallOfSound I think the Tag approach sounds promising just needs some grammar rules for multiple cases like partial and optional.
I'll check out the string approach tomorrow back in office

[zewa666]

@MarshallOfSound ok I've tried out your last comments approach, which fails with:

index.d.ts(1759,23): error TS2314: Generic type 'Partial' requires 1 type argument(s).

and creates this as a resulting d.ts

/**
 * Resizes and moves the window to the supplied bounds. Any properties that are not
 * supplied will default to their current values.
 */
setBounds(bounds: Partial, animate?: boolean): void;

Note the lack of the generic type argument.

---

Looking at the resulting electron-api.json I can see this entry for setBounds though:

{
        "name": "setBounds",
        "signature": "(bounds[, animate])",
        "description": "Resizes and moves the window to the supplied bounds. Any properties that are not supplied will default to their current values.",
        "parameters": [
          {
            "name": "bounds",
            "type": "Partial",
            "collection": false,
            "innerType": "Rectangle",
            "required": true
          },
          {
            "name": "animate",
            "type": "Boolean",
            "collection": false,
            "required": false
          }
        ]
      },

so it does map the type to Partial but properly puts Rectangle into innerType.

So if I'm not mistaken, electron-typescript-defintions doesn't handle the case of innerType + collection: false.

Adding this quickfix inside the conditional checks solves the issue:

} else if (originalType.type === "Partial" && type.innerType) {
      newType = `Partial<${type.innerType}>`
}

and I receive a proper d.ts

/**
 * Resizes and moves the window to the supplied bounds. Any properties that are not
 * supplied will default to their current values.
 */
setBounds(bounds: Partial<Rectangle>, animate?: boolean): void;

Now as said this is just a hacky quickfix but something along these lines should do the trick for your recommended approach for now until there is better support via Tags or custom PartialTypes.

[vladimiry]

@BinaryMuse the issue is being labeled with documentation label but it doesn't seem to be documentation problem. Documentation is correct but typescript declaration is wrong.

[MarshallOfSound]

@vladimiry The TS defs are generated from our docs, if the defs are wrong it's a documentation issue.

[vladimiry]

Got it. It's just a little misleading seeing just "documentation" label here while the solution requires fixing the code (presumably scripts that generate the ts-defs from the docs). PS would love to see Partial<Rectangle> in the ts-defs 😄

[zewa666]

Ouch thats quite long ago. My feature story at that time got abandoned so I didnt follow up on this any longer. @MarshallOfSound did you make any progress with your docs-gen rewrite?

[MarshallOfSound]

@zewa666 The rewrite has landed and Partial<Rectangle> should be super easy to support now 👍

Case and point, here's a PR --> #19416