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

API: Map.View.animate #286

Open
drauggres opened this issue Feb 21, 2020 · 4 comments
Open

API: Map.View.animate #286

drauggres opened this issue Feb 21, 2020 · 4 comments

Comments

@drauggres
Copy link
Contributor

Hello there.
I'm still going through documentation and APIs in order to build compelte and correct TypeScript definitions for Titanium SDK and its modules.

The problem for today is API inconsistency between Map.View and its parent class Ti.UI.View:

namespace Titanium {
  namespace UI {
    class View extends Titanium.Proxy {
      animate(animation: Titanium.UI.Animation | Dictionary<Titanium.UI.Animation>, callback?: (param0: any) => void): void;
    }
  }
}

namespace Map {
  class View extends Titanium.UI.View {
    animate: boolean;
  }
}

It is impossible (in TypeScript) to override method with property.
The best solution would be to rename Map.Views property (for example to animated), otherwise we can't create correct/full definitions for ti.map module.

@drauggres
Copy link
Contributor Author

@sgtcoolguy
@janvennemann
Sorry for call you guys, but this issue might seem as nonsense for other, but you, I think, know what I'm after.

@janvennemann
Copy link

AFAIK the typescript generator has a hacky workaround for this issue, but i guess that breaks the creation dictionary since it doesn't include the animate property now?

I agree with you, we should rename this property so it doesn't conflict with the animate method from the view proxy. However, this would be considered a breaking change and ti.map is a quite popular module. So i was wondering if we can solve this without a breaking change at first. I was thinking of the following:

  • Add the animated property to resolve the name conflict, but keep the animate property for now. Natively we would just treat this as an alias and assign the value of animated to animate. Add a deprecation warning that animate will be replaced with animated in the next major release.
  • The Typescript generator currently ignores animate completely. Maybe we could patch the generation of the creation dictionary so it includes animate?

@drauggres
Copy link
Contributor Author

drauggres commented Mar 6, 2020

Definitions for modules shouldn't be included in titanium.d.ts, they should go into separate files.
So basically we don't provide types for ti.map at the moment, but I generated @ti.types/ti.map for internal usage (and I had to comment out animate property).

There are no workarounds in the generator and it can't generate correct definitions for external modules (but that's another question).

As for the ugly hacks for typescript definitions, I just came up with this:

type MapUglyHackDict = { animate?: boolean } & Omit<Dictionary<Map.View>, 'animate'>;

declare class Map extends Titanium.Module {
  static createView(parameters?: MapUglyHackDict): Map.View;
  // ...
}

But this will help only for view creation, not for changing value later.

@janvennemann
Copy link

Ah crap, you are right. I was thinking about this line here:

https://github.com/appcelerator/docs-devkit/blob/774051ddeed275dcb18f7e3c132b5056e947ab5e/packages/titanium-docgen/generators/typescript_generator.js#L863-L866

But that's just a leftover from the early days when i wrote the typescript generator and it would include definitions for all modules that we bundle with the SDK.

Ok, so now that i'm back on track let me give this another thought and i'll get back to you.

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