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

Export types #529

Closed
loopezz opened this issue Sep 29, 2020 · 5 comments
Closed

Export types #529

loopezz opened this issue Sep 29, 2020 · 5 comments
Labels
feature request New feature request typescript Relate to the plugin's Typescript definition (NOT Ionic Native)

Comments

@loopezz
Copy link

loopezz commented Sep 29, 2020

Feature request

Export types for type hinting for better TypeScript support. Exporting these interfaces should be nice:
https://github.com/dpa99c/cordova-plugin-firebasex/blob/master/types/index.d.ts

@dpa99c
Copy link
Owner

dpa99c commented Oct 3, 2020

Do you mean literally prefixing each interface with export, i.e. export interface Foo ?

@dpa99c dpa99c added feature request New feature request typescript Relate to the plugin's Typescript definition (NOT Ionic Native) labels Oct 3, 2020
@dpa99c dpa99c closed this as completed in 5e349af Oct 3, 2020
@dpa99c dpa99c added the ready for release Something has been implemented and is awaiting release to npm label Oct 3, 2020
@dpa99c dpa99c removed the ready for release Something has been implemented and is awaiting release to npm label Oct 10, 2020
@kamilbrk
Copy link

kamilbrk commented Oct 27, 2020

I'm afraid that this broke existing integrations and it's inconsistent with other Cordova plugins

Without export statements:

Screenshot 2020-10-27 at 13 09 44

With export statements (since 5e349af):

Screenshot 2020-10-27 at 13 10 18

As an example, StatusBar plugin does not use it https://github.com/apache/cordova-plugin-statusbar/blob/master/types/index.d.ts

Not quite sure what the preferred approach is for all Cordova plugins, there doesn't seem to be big of a consensus, but from what I've seen they are usually not using exports and usually theWindow interface gets extended (in addition to declare var).

Those seem to work for StatusBar and other official plugins on a global object without the need to import anything, since default TypeScript tsconfig pulls definitions from underlying node modules and all "types" from across all package.json files. I'd be very thankful for any suggestions for consistency, what's your approach @loopezz?

Also, before I've discovered built-in types I started working on this https://gist.github.com/kamilbrk/546e46dd93a34c4f63022ebdc98459fd and depending on time I'll be most likely working on a PR, although I don't know if you'd like to keep it up to date going forward.

@loopezz
Copy link
Author

loopezz commented Oct 27, 2020

I have made a wrapper service for better integration with Angular and TypeScript. I converted almost all functions to real promises:
Screenshot 2020-10-27 at 15 23 52

To get this type safety I have added FirebasePlugin to my typings.d.ts:
Screenshot 2020-10-27 at 15 24 25


However, your issue is about how these interfaces are declared. You should pass the required arguments for getToken(). If you don't have any callbacks for getToken(), then just do the following:

window.FirebasePlugin.getToken(() => {}, () => {});

@kamilbrk
Copy link

kamilbrk commented Oct 27, 2020

However, your issue is about how these interfaces are declared. You should pass the required arguments for getToken(). If you don't have any callbacks for getToken(), then just do the following:

window.FirebasePlugin.getToken(() => {}, () => {});

Apologies, maybe that was not too clear.

The first screenshot meant that I can get type definitions and it actually tells me that I need callbacks for getToken(), meaning that typings were loaded correctly. I realise that they need to be supplied, it was to demonstrate the point of IDE detecting missing arguments (callbacks).

The second screenshot meant that it couldn't detect any typings whatsoever, so it highlighted FirebasePlugin as unknown. This is something that does not happen for other (core) plugins from my experience, perhaps because of adding export statements.

To get this type safety I have added FirebasePlugin to my typings.d.ts:

interface Window {
  FirebasePlugin: import('../cordova/node_modules/cordova-plugin-firebasex').FirebasePlugin;
}

The fact that you had to add this code to your own typings.d.ts probably demonstrates the reason why other Cordova plugins (mostly core/under Apache org) do that same thing in their own definitions. My understanding is that adding:

interface Window {
  FirebasePlugin: FirebasePlugin;
}

…into this plugin's typings/index.d.ts would mean that you wouldn't have to do this import by yourself. At least that's the behaviour I can observe with other plugins.

Could you share a thought on how do you work with other plugins, perhaps official/core plugins from Apache org on GitHub? Do you also import them into your project? Do they work out of the box without importing?

@peitschie
Copy link

We've hit this one as well.

It turns that putting the export on the interfaces makes TypeScript decide this is a Module now rather than a Namespace: https://www.typescriptlang.org/docs/handbook/modules.html

In TypeScript, just as in ECMAScript 2015, any file containing a top-level import or export is considered a module. Conversely, a file without any top-level import or export declarations is treated as a script whose contents are available in the global scope (and therefore to modules as well).

The easiest way to fix is this move the declare var within a global scope like this:

declare global {
    const FirebasePlugin: FirebasePlugin;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature request typescript Relate to the plugin's Typescript definition (NOT Ionic Native)
Projects
None yet
Development

No branches or pull requests

4 participants