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

Add TypeScript definition #68

Merged
merged 7 commits into from
Jan 28, 2019
Merged

Add TypeScript definition #68

merged 7 commits into from
Jan 28, 2019

Conversation

CvX
Copy link
Contributor

@CvX CvX commented Jan 27, 2019

Closes #66

index.d.ts Outdated
*
* @param window - Default: BrowserWindow.getFocusedWindow()
*/
export function refresh(window?: Electron.BrowserWindow): void;
Copy link
Owner

@sindresorhus sindresorhus Jan 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you use a default parameter here?

export function refresh(window?: Electron.BrowserWindow = BrowserWindow.getFocusedWindow()): void;

Then line 43 would be moot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the default value doesn't seem to show up in the inline docs preview when using this syntax.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it’s valid syntax, right? If so, I think we should still use it and open an issue on VSCode about it.

@sindresorhus
Copy link
Owner

Instead of using the Electron global, wouldn't it be better to explicitly import what we need?

import {BrowserWindow} from 'electron';

index.d.ts Outdated
* win = new BrowserWindow();
* });
*/
export default function electronDebug(options?: Readonly<Options>): void;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think we should use readonly on the specific properties whenever possible, as Readonly will show up in the code completion and create noise: screen shot 2019-01-28 at 19 56 25

Maybe that's a non-issue. I dunno.

Thoughts?


*Really want the readonly type operators...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure either…
Is that information useful in any way to the end user of the API? If it's not, then sure, readonly all the props. 😉

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not. It’s only useful for us to ensure we don’t modify the object.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s go with per-property annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Really want the readonly type operators...

Btw. TS seems to be slowly closing in on that… 😅
microsoft/TypeScript#29435
microsoft/TypeScript#29510

Not quite there yet, but I like the trajectory. 😉

index.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit a1ac261 into sindresorhus:master Jan 28, 2019
@sindresorhus
Copy link
Owner

#68 (comment) doesn't actually work...

I'm getting the error:

TS2371: A parameter initializer is only allowed in a function or constructor implementation.

Seems it's not allowed in ambient context... Which sucks.

I ended up with this: 142b2de#diff-b52768974e6bc0faccb7d4b75b162c99R52

I tried @param [window=BrowserWindow.getFocusedWindow()], which is documented in the JSDoc docs, but that doesn't show anything in VSCode...

@sindresorhus
Copy link
Owner

I also realized another thing. There actually isn't really any point in using readonly for the input options after all. Seeing as the source where we use the options object is in JS, so the type definition isn't actually enforcing the read-only-ness... So it feels moot, unless I'm missing something?

@CvX
Copy link
Contributor Author

CvX commented Jan 28, 2019

It basically acts as a blueprint for the potential conversion to TypeScript. Or a source of "truth" about the code, even if not enforced by the tooling. 🤷‍♂️

@sindresorhus
Copy link
Owner

Hmm, yeah, I guess. Alright, let's keep it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants