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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Electron's new official TypeScript Definitions #12888

Closed
3 tasks done
zeke opened this issue Nov 23, 2016 · 15 comments
Closed
3 tasks done

Using Electron's new official TypeScript Definitions #12888

zeke opened this issue Nov 23, 2016 · 15 comments

Comments

@zeke
Copy link

zeke commented Nov 23, 2016

  • I have a question that is inappropriate for StackOverflow.
  • I want to talk about electron/electron.d.ts
  • The authors of that type definition are cc'd @vvakame @rhysd @miniak

馃憢

I work on the Electron team at GitHub. We are in the process of generating an electron.d.ts file as part of our release process, and will soon be bundling it in the electron (and electron-prebuilt) npm package. electron-userland/electron-prebuilt#209

I am attempting to remove the existing Electron definitions from this repository to prevent issues/collisions for existing users of the third-party definition file. I'm following the steps outlined at https://github.com/DefinitelyTyped/DefinitelyTyped#removing-a-package, but I do not see a notNeededPackages.json file as mentioned in those instructions. Am I missing something?

cc @MarshallOfSound @niik

@niik
Copy link
Contributor

niik commented Nov 23, 2016

but I do not see a notNeededPackages.json file as mentioned in those instructions. Am I missing something?

I think that's because you're looking at the master branch but all definitions that doesn't target typings live in the types-2.0 branch. See https://github.com/DefinitelyTyped/DefinitelyTyped/blob/types-2.0/notNeededPackages.json

@rhysd
Copy link
Contributor

rhysd commented Nov 24, 2016

@zeke

Hi, that's great to see the official TypeScript support for Electron!

Alhtough this should not be discussed here, but I have some concerns about removing current definition file immediately. I want to know that the new type definition actually can replace electron/index.d.ts in this repo.

  • Is the new type definition well-tested? I think electron-main-tests.ts and electron-renderer-tests.ts are available for it (although it doesn't cover all of APIs)
  • The type definition exports proper definitions? Some types (e.g. Type of callback of IPC events, interface for WebContents objects, ...) should be imported from outside to be used in user code.

I want to see the generated type definition to know that it has enough quality to replace type definition in DefinitelyTyped.

@MarshallOfSound
Copy link

@rhysd I'm currently in the process of moving city so my computer is currently in a box 馃槩

When I unpack though I'll test the generated definition against those files and ensure it works 馃憤

FWIW I have already tested the definition against a few typescript projects I have and against VS Code and all of them required minimal changes to work with the new definitions (10 - 40 changes) mostly due to type names changing. I.e. In the current definition we have Electron.ShowOpenDialogOptions and in the generated one we have Electron.OpenDialogOptions. Small names changes like that are a result of the auto-generation of the typescript definition and take a few minutes to update across your project.

If you want to take a look at the definition before I get setup again (or before @zeke sees this) you can just clone https://github.com/electron/electron-definitelytyped and run npm run demo and it will generate you an electron.d.ts file. 馃憤

@MarshallOfSound
Copy link

MarshallOfSound commented Nov 25, 2016

@rhysd So I borrowed someones laptop and took a look. With the latest docs and the latest generator I get 3 errors on the main tests and like 8 on the renderer ones (The renderer errors are only webview related).

The renderer ones are to be expected as we don't export the webview usage as an actual API in our JSON file. And the main tests appear to be due to the WebRequest doc not be parsed at all.

Will probably need to discuss how best to handle the WebView API stuff somewhere (slack maybe). I have a sneaky idea that we can just inherit from WebContents. Hit me up on Slack if you have questions / want to discuss it 馃憤

EDIT: Link to generated file --> https://gist.github.com/MarshallOfSound/8b1b1aa0773fab1a9178cc2d1025dadd

@rhysd
Copy link
Contributor

rhysd commented Nov 29, 2016

@MarshallOfSound Thank you for detailed explanation. It sounds nice 馃憤 I'll also try it in my apps.

@MarshallOfSound
Copy link

@rhysd @niik Want to try get the ball rolling on this again. The linked gist has been updated with the latest generated typings:

https://gist.github.com/MarshallOfSound/8b1b1aa0773fab1a9178cc2d1025dadd

And now passes all of the tests currently in this repository.

Looking for the best way forward RE moving towards a typings file that we publish rather than the manually maintained one here. We are almost at the point where we can ship an electron.d.ts file with each of our Electron releases (just need to do some wiring in our releaser). Few outstanding questions though.

@miniak
Copy link
Contributor

miniak commented Apr 3, 2017

I would like to review the auto-generated file thoroughly. Last time I've checked (two weeks ago), there were still some issues.

@zeke
Copy link
Author

zeke commented Apr 3, 2017

@miniak that would be great.

Please report anything you find at https://github.com/electron/electron-typescript-definitions/issues, and note that there are already a few issues filed there.

@joshaber
Copy link
Contributor

joshaber commented Apr 4, 2017

Is there a way we can publish our typings to the @types namespace? electron/typescript-definitions#10

Yep, submit a PR to https://github.com/DefinitelyTyped/DefinitelyTyped. But you shouldn't need to do that. See below.

What's the formal process for removing these typings / forwarding them to our typings?

TypeScript will prefer the typing we include in Electron over any typings provided by @types. So users won't need to do anything with @types. So long as the typing is in Electron and pointed to by the package.json It'll Just Work庐.

The trick will be communicating to users that they no longer need @types for Electron and can remove that dependency.

@zeke
Copy link
Author

zeke commented Apr 4, 2017

users won't need to do anything with @types

It was suggested (by @felixrieseberg, I believe) that people should still have a way to install the official Electron definitions without having to include all of electron as a dependency.

@zeke zeke changed the title Removing Electron TypeScript Definitions Using Electron's new official TypeScript Definitions May 17, 2017
@zeke
Copy link
Author

zeke commented May 17, 2017

We're just about to ship an official electron.d.ts file bundled with the electron npm package. It's currently available in a beta. To try it, run npm i electron@1.7.1 or npm i electron@beta.

WIP blog post: electron/electronjs.org-old#731

As I mentioned earlier, we can continue publishing the new auto-generated types here as @types/electron, as folks may find it useful to sometimes use Electron's type definitions without also depending on electron itself.

@zeke
Copy link
Author

zeke commented May 17, 2017

@miniak if you can help update the typings here, that would be awesome!

You also mentioned updating the typings registry. Is that still something you can take care of?

@zeke
Copy link
Author

zeke commented May 24, 2017

cc @DanielRosenwasser

@DanielRosenwasser
Copy link
Member

The trick will be communicating to users that they no longer need @types for Electron and can remove that dependency.

The types publisher has a mechanism for this. For example, if I npm install @types/redux, I'll get...

WARN deprecated @types/redux@3.6.0: This is a stub types definition for Redux (https://github.com/reactjs/redux). Redux provides its own type definitions, so you don't need @types/redux installed!

I don't know the extent to which you'll need to worry about the registry. If it's in the package itself you should be fine.

@zeke zeke mentioned this issue Jun 6, 2017
3 tasks
@callumlocke
Copy link

Can this issue be closed yet?

@zeke zeke closed this as completed Jan 9, 2020
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

No branches or pull requests

8 participants