Skip to content

Conversation

drauggres
Copy link
Contributor

@drauggres drauggres commented Dec 18, 2019

  • Dictionary (typed factory methods):
const button = Ti.UI.createButton({
 width: 100,
 backgroundColor: '#ddd',
 text: '+',   // TS2345: [..] 'text' does not exist in type [..]
 left: 10
});
  • Typed this in addEventListner, removeEventListener:
const button = Ti.UI.createButton();
button.addEventListener('click', function(e) {
 this.backgroundColor = 'red';
 this.text = 123;  // TS2339: Property 'text' does not exist on type 'Button'.
});
  • Types for Events:
const button = Ti.UI.createButton();

// know event type
button.addEventListener('click', function(e) {
 e.source.backgroundColor = 'red';
 console.log(`x: ${e.x}`);
 console.log(`x: ${e.z}`);  // TS2339: Property 'z' does not exist on type 'Button_click_Event'.
});

// unknown event type
button.addEventListener('myEvent', function(e) {
 console.log(e.lol);  // ok, because 'myEvent' is unknown event type -> e: any
});
  • Modules declared as both namespace and class (with static methods). This allows to define types for specific events for Modules like Ti.Geolocation, Ti.Media, Ti.Network etc.:
Ti.Network.addEventListener('change', e => {
  console.log(e.reason);
  console.log(e.status);  // TS2339: Property 'status' does not exist on type 'Network_change_Event'
});

Additonal changes:

  1. inject id?: string|number in Ti.Proxy
  2. rest parameter for Titanium.Database.DB.execute

Explanations:

  1. For every event generated:
  • base event interface with name ${ClassName}BaseEvent (e.g. ButtonBaseEvent)
  • event interface with name ${ClassName}_${EventName}_Event (e.g. Button_click_Event) and extended from the base event
  • "map" interface to math event name and event interface (e.g. ButtonEventMap)
  • note: : in the name of a event is replaced with _ for generated interfaces (event name itself is not changed)

IMPORTANT: event interface name is not documented and is generated as shown above. Since they will be in users code (if they decide to write in TypeScript), they could not be changed at any time. These names will be part of the API and must remain stable.

  1. removed canExtend + removeMembers: were not really useful, only could decrease size of the output, but removeMembers did not take in accout excludes, so result was incorrect.
  2. removed generateMethodOverloadsIfRequired: as was mentioned in the function description:

Currently only handles one case where a parameter can be passed as both a single value and as an array, e.g. Ti.UI.View, Array<Ti.UI.View>

TypeScript can handle this case e.g. add(view: Titanium.UI.View | Titanium.UI.View[]): void;

  1. code from nodes constructors moved to the init method: since for one module could be generated two nodes (interface and namespace) in order to decrease output size I'm skipping namespace node without any internal interfaces and namespaces. To do this I need at first to build "the tree" and then deside where I want to keep properties and methods/functions.

Only thing that makes impossible to use generated file to write in TypeScript is the problem with PickerColumn

UPD: APIs from files under apidoc/Modules are removed from output. It is incorrect to put them inside of titanium/index.d.ts (ideally they should be placed in separate files ti.cloud/index.d.ts and ti.cloudpush/index.d.ts).

More things came up:

@drauggres
Copy link
Contributor Author

Here is the result for this + #24 + tidev/titanium-sdk/pull/11348

@drauggres
Copy link
Contributor Author

Tested file from the comment above with DefinitelyTyped linter. Work fine. Even found an error in the tests.

- remove trailing spaces from jsdoc
- use "any[]" as parameter for Ti.Database.DB.execute
- use "Array<Dictionany<T>>" type instead of "Dictionary<T>[]"
- add "tslint:disable-next-line:ban-types" for "Function" type
- update TypeScript Version to 3.0: "excluded" methods are typed as properties
 with "methodName: never", this is not allowed before 3.0
generated class for Titanium will have only static members
It is valid to pass a function (e.g. "onload") to Ti.Network.createHTTPClient()
in Dictionary<Ti.Network.HTTPClient>.
So now we exclude from T only Functions that came from Ti.Proxy
Since SDK 9.0 accessors will be removed, so they will not bother us here.
@janvennemann janvennemann self-requested a review January 10, 2020 14:14
@drauggres
Copy link
Contributor Author

Last changes:

  • Remove from output APIs from files under apidoc/Modules. It is incorrect to put them inside of titanium/index.d.ts (as Ti.Modules.<Name>): they should be placed in separate files ti.cloud/index.d.ts and ti.cloudpush/index.d.ts (TBD, I have no plans on this).
  • Add @deprecated annotation and deprecation notes into jsdoc for deprecated APIs.

Now (after tidev/titanium-sdk#11348 is merged) it is possible to generate valid definitions with this generator.

I think, I'm finally done here. @janvennemann please review.

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

@drauggres First of all, a big thank you for porting this to the existing typescript generator. The typed create functions and events are super awesome!

I did a first pass through this and added a couple of notes. I also have a question about one of the things you removed:

removed canExtend + removeMembers: were not really useful, only could decrease size of the output, but removeMembers did not take in accout excludes, so result was incorrect.

Instead of just ripping it out completely, why not improve it to respect excludes? Your generated output is twice the size of how it was before. Did you test it with excludes to see if it's worth it?

@drauggres
Copy link
Contributor Author

Instead of just ripping it out completely, why not improve it to respect excludes? Your generated output is twice the size of how it was before. Did you test it with excludes to see if it's worth it?

We don't need canExtend all information about inheritance is already present in __inherits property.
This and this will reduce size of the output (so it will be about 32% larger than original).

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

@drauggres Awesome, the size looks much better now.

I copied the generated types with this PR over to the types in DefinitelyTyped and tried to run npm run lint titanium to see if it validates. It's spitting out errors about duplicate identifiers console and JSON. Can you check that? Removing dom gets rid of the console error. But JSON seems to be always included, even with es5 only. Do we need to exclude that in our typings?

The only remaining thing i'd really like to change are the event names to have a consistent camel-cased naming scheme, but since that is not possible due to technical limitations i guess we have to go with the underscored names.

@drauggres
Copy link
Contributor Author

I didn't try to run the linter after this comment (only compiled my projects with new definitions).

Removing dom gets rid of the console error.

Work fine for me even with dom (my DefinitelyTyped fork). If linter fails with dom we can exlude it; compliation with dom included works fine (as you know, it is required by backbone).

But JSON seems to be always included, even with es5 only.

It looks like we can't (and probably shouldn't) declare JSON variable and interface in global space, because it's part of the ECMA-262 5.1 specification (and always exists in es5+ environment). Solutions:

  1. Skip JSON in generator
  2. Remove JSON.yml from apidoc (and JSON property from Global.yml)

Also, because I added @deprecated tag, linter complains about violation of no-redundant-jsdoc-2 rule. Solutions:

  1. Add "no-redundant-jsdoc-2": false into tslint.json
  2. Remove @deprecated tag

@janvennemann
Copy link
Contributor

Removing dom gets rid of the console error.

Work fine for me even with dom (my DefinitelyTyped fork). If linter fails with dom we can exlude it; compliation with dom included works fine (as you know, it is required by backbone).

I see the following error when including dom lib and @types/titanium together in a project. Note that they only pop up when i actually try to compile using tsc, linting in VSCode works fine.

node_modules/@types/titanium/index.d.ts:1152:1 - error TS1046: Top-level declarations in .d.ts files must start with either a 'declare' or 'export' modifier.

1152 class console {
     ~~~~~

node_modules/@types/titanium/index.d.ts:1152:7 - error TS2300: Duplicate identifier 'console'.

1152 class console {
           ~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:19661:13
    19661 declare var console: Console;
                      ~~~~~~~
    'console' was also declared here.

node_modules/typescript/lib/lib.dom.d.ts:19661:13 - error TS2300: Duplicate identifier 'console'.

19661 declare var console: Console;
                  ~~~~~~~

  node_modules/@types/titanium/index.d.ts:1152:7
    1152 class console {
               ~~~~~~~
    'console' was also declared here.


Found 3 errors.

So we can't use both together or it will blow up. But we also can't exclude our global console, because then it would be missing if dom lib is not included.

This is kind of a mess, especially when using TypeScript in Alloy (as it uses backbone). Please see some related discussion here and here. I haven't come up with a proper solution for this yet, so any ideas are greatly appreciated. IMHO, including dom in Titanium projects is wrong, since we don't support 99% of the types that are defined there. The only possible fix for this that i see is that we fork our own versions of the backbone types and rip out all types that create the dependency on the dom lib.

But JSON seems to be always included, even with es5 only.

It looks like we can't (and probably shouldn't) declare JSON variable and interface in global space, because it's part of the ECMA-262 5.1 specification (and always exists in es5+ environment). Solutions:

  1. Skip JSON in generator
  2. Remove JSON.yml from apidoc (and JSON property from Global.yml)

Agreed, we should remove JSON from our docs since it's part of the ECMA spec. Users can refer to JSON on MDN for example.

Also, because I added @deprecated tag, linter complains about violation of no-redundant-jsdoc-2 rule. Solutions:

  1. Add "no-redundant-jsdoc-2": false into tslint.json
  2. Remove @deprecated tag

Ok, that's odd. I don't see those linting issues. These are the only linting issues is see once i resolve the above issues (removed dom lib and removed JSON types):

~/Development/typescript/DefinitelyTyped on master ❱❱❱ npm run lint titanium

> definitely-typed@0.0.3 lint /Users/jvennemann/Development/typescript/DefinitelyTyped
> dtslint types "titanium"

Error: /Users/jvennemann/Development/typescript/DefinitelyTyped/types/titanium/index.d.ts:1152:1
ERROR: 1152:1  no-unnecessary-class  Every member of this class is static. Use namespaces or plain objects instead.
ERROR: 1152:1  expect                TypeScript@next compile error: 
Top-level declarations in .d.ts files must start with either a 'declare' or 'export' modifier.

/Users/jvennemann/Development/typescript/DefinitelyTyped/types/titanium/titanium-tests.ts:35:4
ERROR: 35:4    expect                TypeScript@next compile error: 
Type 'string' is not assignable to type 'never'.

    at /Users/jvennemann/Development/typescript/DefinitelyTyped/node_modules/dtslint/bin/index.js:193:19
    at Generator.next (<anonymous>)
    at fulfilled (/Users/jvennemann/Development/typescript/DefinitelyTyped/node_modules/dtslint/bin/index.js:6:58)v

@drauggres
Copy link
Contributor Author

I see the following error when including dom lib and @types/titanium together in a project. Note that they only pop up when i actually try to compile using tsc, linting in VSCode works fine.

node_modules/@types/titanium/index.d.ts:1152:1 - error TS1046: Top-level declarations in .d.ts files must start with either a 'declare' or 'export' modifier. 
1152 class console {

There is no class console in the output from this version of the generator.

Ok, that's odd. I don't see those linting issues.

For some reason they don't come up when I run linting in DefinitelyTyped repo clone, only in my fork (probably there are different versions of some packages). Nevermind.

These are the only linting issues is see once i resolve the above issues (removed dom lib and removed JSON types):

ERROR: 1152:1  no-unnecessary-class  Every member of this class is static. Use namespaces or plain objects instead.
ERROR: 1152:1  expect                TypeScript@next compile error: 
Top-level declarations in .d.ts files must start with either a 'declare' or 'export' modifier.

Line 1152 is class console, it shouldn't be there.

ERROR: 35:4    expect                TypeScript@next compile error: 
Type 'string' is not assignable to type 'never'.

    at /Users/jvennemann/Development/typescript/DefinitelyTyped/node_modules/dtslint/bin/index.js:193:19
    at Generator.next (<anonymous>)
    at fulfilled (/Users/jvennemann/Development/typescript/DefinitelyTyped/node_modules/dtslint/bin/index.js:6:58)v

That is a valid error: url property was removed from Ti.UI.ImageView long time ago.

@janvennemann
Copy link
Contributor

There is no class console in the output from this version of the generator.

Sry, my bad, it seems like i was running an outdated version of your branch. Regenerated it and now the error is gone.

ERROR: 35:4    expect                TypeScript@next compile error: 
Type 'string' is not assignable to type 'never'.

    at /Users/jvennemann/Development/typescript/DefinitelyTyped/node_modules/dtslint/bin/index.js:193:19
    at Generator.next (<anonymous>)
    at fulfilled (/Users/jvennemann/Development/typescript/DefinitelyTyped/node_modules/dtslint/bin/index.js:6:58)v

That is a valid error: url property was removed from Ti.UI.ImageView long time ago.

Duh, of course! About time to update our tests inside DefinitelyTyped, which is where i got that from.

Everything seem to be sorted out now, approved! Awesome work man!

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