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

Discuss: Replace implicit return type with explicit and void return? #38

Closed
tlancina opened this issue Mar 10, 2016 · 3 comments
Closed

Comments

@tlancina
Copy link
Contributor

Right now many of the wrapper functions return something along the lines of:

    // This Promise is replaced by one from the @Cordova decorator that wraps
    // the plugin's callbacks. We provide a dummy one here so TypeScript
    // knows that the correct return type is Promise, because there's no way
    // for it to know the return type from a decorator.
    // See https://github.com/Microsoft/TypeScript/issues/4881
    return new Promise<any>((res, rej) => {});

With the intention to reduce confusion about the decorator magic that is happening to change the return type. But I'm wondering if we just add the return type and a void return, so each wrapper looks more like an interface:

  @Cordova()
  static show(options?: {
    buttonLabels: string[],
    title?: string,
    androidTheme?: number,
    androidEnableCancelButton?: boolean,
    winphoneEnableCancelButton?: boolean,
    addCancelButtonWithLabel?: string,
    addDestructiveButtonWithLabel?: string,
    position?: number[]
  }) : Promise<any> { return }


  /**
   * Hide the ActionSheet.
   */
  @Cordova()
  static hide() : Promise<any> { return }

Seems a bit cleaner, @ihadeed thoughts?

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 10, 2016

That would make the definitions a lot cleaner.

Wouldn't it also help if we ensure that every promise has a defined type
--example, like in geolocation.ts : Promise<Geoposition>, instead of
Promise<any> ? I think it will help the docs generation process as well.

@tlancina
Copy link
Contributor Author

Yeah I think if possible we should try and have the wrapper be as small as possible, and use interfaces for the option objects as well:

  /**
   * Show the ActionSheet.
   */
@Cordova()
  static show(options?: ActionSheetOptions) : Promise<any> { return }

  /**
   * Hide the ActionSheet.
   */
  @Cordova()
  static hide() : Promise<any> { return }

I also am not a huge fan of having the interfaces cluttering up the top of the files, it would be nice to have just the description of the wrapper, with the very concise method definitions. I can't remember if there is a good way to move them into their own files, while still using declaration: true. I want to say we have to concatenate them manually as part of the build.

Regarding the promises, I agree, if possible they should all have the correct type.

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 10, 2016

Is it something like this that you're looking for : https://www.npmjs.com/package/declaration-bundler-webpack-plugin ?

tlancina added a commit that referenced this issue Mar 10, 2016
tlancina added a commit that referenced this issue Mar 10, 2016
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

2 participants