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 #11

Merged
merged 18 commits into from
Jan 29, 2019
Merged

Add TypeScript definition #11

merged 18 commits into from
Jan 29, 2019

Conversation

CvX
Copy link
Contributor

@CvX CvX commented Jan 28, 2019

[WIP] There are still a couple of TODOs to be resolved.

edit:
🚀

@sindresorhus
Copy link
Owner

Can you mark properties that are meant as read-only with readonly and if all are readonly, you could use the Readonly helper.

https://basarat.gitbooks.io/typescript/docs/types/readonly.html

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
/**
* TODO
*/
url: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
url: string;
url?: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@CvX
Copy link
Contributor Author

CvX commented Jan 29, 2019

@sindresorhus Correct me if I'm wrong, but it seems that there's a type incompatibility here.

Both openUrlMenuItem() and aboutMenuItem() create and return instances of Electron.MenuItem. In their examples/README (and in Caprine) these are used in Electron.Menu.buildFromTemplate([]) as either top-level items or in submenu.

Those accept MenuItemConstructorOptions:

submenu?: (MenuItemConstructorOptions[]) | (Menu);
static buildFromTemplate(template: MenuItemConstructorOptions[]): Menu;

These are not compatible with MenuItem. I believe it currently works just because the MenuItem constructor assigns all properties from options, so when options are another MenuItem instance, it essentially copies it?

@sindresorhus
Copy link
Owner

Wow. Good catch. Let's change them to return MenuItemConstructorOptions.

@sindresorhus
Copy link
Owner

I'm gonna open an issue tomorrow about Electron supporting MenuItem too in the template builder, but that will probably take a long time, if ever.

@CvX
Copy link
Contributor Author

CvX commented Jan 29, 2019

Wow. Good catch. Let's change them to return MenuItemConstructorOptions.

That's TypeScript at work! 😄I've been testing these type definitions inside Caprine's codebase, so it got flagged instantly. 🙂

index.d.ts Outdated Show resolved Hide resolved
index.test-d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 78c63f6 into sindresorhus:master Jan 29, 2019
@sindresorhus
Copy link
Owner

Woah. This was a massive undertaking and you did an amazing job at it! 🎉

@sindresorhus
Copy link
Owner

@sindresorhus
Copy link
Owner

I'm gonna open an issue tomorrow about Electron supporting MenuItem too

Done: electron/electron#16645

Omit<T, Keys>
& {
[K in Keys]-?: Required<Pick<T, K>>
}[Keys]
Copy link
Owner

Choose a reason for hiding this comment

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

@CvX Would you be willing to submit this to https://github.com/sindresorhus/type-fest ? I'm trying to consolidate the types I use so they can be properly documented and tested in one place.

CvX added a commit to CvX/type-fest that referenced this pull request Mar 13, 2019
CvX added a commit to CvX/type-fest that referenced this pull request Mar 13, 2019
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