Add TypeScript declarations. #4928

aomarks · 2017-11-10T20:27:52Z

Generated by https://github.com/Polymer/gen-typescript-declarations. Regenerate with gulp generate-typescript. Types are output to a types/ directory.

Fixes #4025

dfreedm

Maybe move all the d.ts files to a types folder?

dfreedm · 2017-11-14T00:33:15Z

gulpfile.js

@@ -267,8 +267,18 @@ gulp.task('generate-externs', ['clean'], () => {
  });
 });

+gulp.task('generate-typescript', () => {
+  let genTs = require('@polymer/gen-typescript-declarations').generateDeclarations;
+  del.sync(['**/*.d.ts']);


Just use promise api with del

(async/await ftw)

justinfagnani · 2017-11-14T20:36:24Z

I think siblings makes sense, especially with the move to modules coming up.

/cc @FredKSchott and @usergenic because we'll need modulizer to re-generate the types.

aomarks · 2017-11-17T05:07:39Z

@justinfagnani I'm fine with that, but note that once we're on modules, we'll also be on NPM, so we can and should add types as part of prepack instead of checking into source.

I think the objection to sibling files from @azakus is that it clutters up the source repo. Siblings for 2.0 is simpler from my perspective, but I understand not wanting to clutter the repo.

Can you elaborate on how this will work with modulizer? I would have thought we'd just blow away the 2.0 types before we run modulizer, and re-run the generator afterwards, once it has support for modules?

TimvdLippe · 2017-11-18T14:33:05Z

I have updated the description to reference #4025 which this PR fixes as well.

Annotate that dedupingMixin returns T.

Includes hand written types for cases that couldn't be generated. Uses https://github.com/Polymer/gen-typescript-declarations. Run with `gulp generate-typescript`. Also asyncify the gulpfile with fs-extra.

aomarks · 2017-12-08T23:40:11Z

Hi all. This is ready for another look. Highly recommend reviewing the 3 commits separately.

Types are now output to a types/ directory instead of as sibling files. We made this decision to avoid cluttering the repo. For Polymer 3 on NPM, we will instead output as siblings as part of prepublish, instead of committing to the repo at all.
Integrates all of the fixes listed at https://github.com/Polymer/gen-typescript-declarations/issues/23. There are still a few missing APIs, but I don't think we should block merging on that. All of the missing APIs are things that Analyzer doesn't know about at all, so they have also never been in our documentation.
Added a commit with some minor JSDoc tweaks.

aomarks · 2017-12-08T23:49:13Z

Also, this now includes the extra-types.d.ts file, which are TypeScript versions of the hand-written types we needed from the Closure externs.

dfreedm

Can we add a note that we now require Node 8 in the README?

Generally LGTM, minus that nit and need to double-check closure with return type on deduping mixin.

dfreedm · 2017-12-08T23:52:51Z

lib/utils/mixin.html

@@ -38,6 +38,7 @@
   * @memberof Polymer
   * @template T
   * @param {T} mixin ES6 class expression mixin to wrap
+   * @return {T}


From what I remember, the magic voodoo that Closure Compiler uses for mixins doesn't like a return type here. Can you double check that gulp lint-closure doesn't produce any warnings?

Can we add a note that we now require Node 8 in the README?

Added some items to the CHANGELOG. Where should I mention the Node version in the README? It doesn't currently mention the gulp script. (async/await was actually 7.6 btw).

From what I remember, the magic voodoo that Closure Compiler uses for mixins doesn't like a return type here. Can you double check that gulp lint-closure doesn't produce any warnings?

I ran gulp lint, and it produces a lot of warnings. Nothing about dedupingMixin that I can see, though. Here's the log, does it look normal?: https://gist.github.com/aomarks/3adb586df000a36e682cc4f87137606b

Closure is compiling cleanly now.

TimvdLippe

I have a couple of questions regarding the return types of several methods.
Most notably, I see several methods that have void|null, where the code has no return statement and therefore should be just void.
Also there are several callbacks that have return type any, but the element definitions do not define a return type for these methods.
(Is there a reason these callbacks are typed anyways, as it seems they are only included if they are specified if the element uses them)

I am not sure if the overall goal is 100% type-correct, but as a TypeScript user I would expect the types to be as specific as possible.
Therefore if methods do not return anything, void is more specific than any.
Since all of the types are generated, I am not sure if this is a bug in the gen-typescript-declarations or that our Closure types are wrong.

To summarize the other points:

Change void|null to void
Change any to void for return types where functions do not return anything
Change X|null to just X (for most of the cases)
Unify all callback declarations to 1 class that everyone extends, probably just in property-accessors.d.ts, which is the lowest entry-point
Some other small comments in relation to specific functions
Remove |null when argument is already optional with X?

Sorry for the many comments, but I am really excited for this change! Just would like to be as specific as possible and not "lie" through our types 😂

TimvdLippe · 2017-12-10T12:57:46Z

types/lib/elements/array-selector.d.ts

+   */
+  function ArraySelectorMixin<T extends new(...args: any[]) => {}>(base: T): {
+    new(...args: any[]): ArraySelectorMixin & Polymer.ElementMixin
+  } & T


This is some nice TypeScript voodo 👌

I think these would be easier to read if you define the Constructor<T> type.

TimvdLippe · 2017-12-10T12:59:30Z

types/lib/elements/array-selector.d.ts

+    /**
+     * Clears the selection state.
+     */
+    clearSelection(): any;


I think this should be void

Done. Added @returns annotation.

TimvdLippe · 2017-12-10T13:00:11Z

types/lib/elements/array-selector.d.ts

+     *
+     * @param item Item from `items` array to deselect
+     */
+    deselect(item: any): void|null;


Should just be void. I do not see a return statement in this method that returns null.

Done. Fixed nullable void bug in Polymer/gen-typescript-declarations#40.

TimvdLippe · 2017-12-10T13:01:33Z

types/lib/elements/dom-bind.d.ts

+     */
+    attributeChangedCallback(): any;
+    connectedCallback(): any;
+    disconnectedCallback(): any;


All these methods are defined on HTMLElement which should be void as well.

Done. Added @returns annotation.

TimvdLippe · 2017-12-10T13:06:19Z

types/lib/elements/dom-module.d.ts

+   *     let img = customElements.get('dom-module').import('foo', 'img');
+   */
+  class DomModule extends HTMLElement {
+    attributeChangedCallback(name: any, old: any, value: any): any;


The name can be string here.

Done. Added @param annotation.

TimvdLippe · 2017-12-10T14:15:45Z

types/lib/utils/flush.d.ts

+  /**
+   * Adds a `Polymer.Debouncer` to a list of globally flushable tasks.
+   */
+  function enqueueDebouncer(debouncer: Polymer.Debouncer|null): void|null;


Can not be null. That would throw an error later in flushDebouncers()

Done. Updated @param annotation.

TimvdLippe · 2017-12-10T14:16:52Z

types/lib/utils/gestures.d.ts

+    /**
+     * Adds an event listener to a node for the given gesture type.
+     */
+    function addListener(node: Node|null, evType: string, handler: Function|null): boolean;


All of these (and in the later methods) can not be |null.

Done. Updated @param annotations.

TimvdLippe · 2017-12-10T14:19:22Z

types/lib/utils/import-href.d.ts

+   * In the `onload` callback, the `import` property of the `link`
+   * element will contain the imported document contents.
+   */
+  function importHref(href: string, onload?: Function|null, onerror?: Function|null, optAsync?: boolean): HTMLLinkElement|null;


For some reason this type is different than the one specified in legacy-element-mixin.d.ts. Duplication FTW?

Done. Made them the same.

TimvdLippe · 2017-12-10T14:32:44Z

types/lib/utils/settings.d.ts

+  /**
+   * Legacy settings.
+   */
+  namespace settings {


We should probably add the declarations for the following:

namespace settings { useNativeCSSProperties: boolean; useNativeCustomElements: boolean; useShadow: boolean; }

I moved the @namespace annotation from settings to Settings so at least that's correct now. But actually there is no way in Analyzer to model this object and its interface. Going to punt on the full interface for now. The main way people use this is with a <script> tag in their index.html anyway, so it's unlikely it would need to be referenced from TypeScript.

TimvdLippe · 2017-12-10T14:33:41Z

types/lib/utils/unresolved.d.ts

+ * To modify these typings, edit the source file(s):
+ *   lib/utils/unresolved.html
+ */
+


Maybe do not generate the file if there were no types in it?

Currently we mirror the HTML import graph to the reference import graph, so that complicates that a bit, since dependents need to know whether their dependency has anything. See also https://github.com/Polymer/gen-typescript-declarations/blob/master/src/gen-ts.ts#L114

aomarks · 2017-12-16T00:12:24Z

PTAL.

This PR is up to date with the latest changes to Polymer, Analyzer, and the type generator.

justinfagnani

I think this is basically ready to merge, so it has my approval. Any nits can be fixed up later, otherwise this could go around forever. I'd like to see it go out with a 2.4.0 prerelease to get feedback on the types if possible.

I left a handful of observations on generated types that I think could be better (like a lot of methods that return any but probably should return void), but I think those can be incrementally improved either in the generator or Polymer itself. Just add issues.

One thing that would be nice is if we could run the generated types through the type checker to make sure they're at least well formed and all referenced built-in types are defined.

justinfagnani · 2017-12-16T02:25:02Z

gulpfile.js

+  const files = await genTs('.', config);
+  for (const [filePath, contents] of files) {
+    await fs.outputFile(path.join('types', filePath), contents);
+  }
 });

 gulp.task('update-version', () => {
  gulp.src('lib/utils/boot.html')


fyi, this task looks like it's missing a return statement. It should return the stream pipeline.

Wasn't changed in this PR, but added a return.

justinfagnani · 2017-12-16T02:26:09Z

types/extra-types.d.ts

+ * Types from "externs/closure-types.js"
+ */
+
+type Polymer_PropertyEffects = Polymer.PropertyEffects;


Why is this necessary? Add a comment?

No longer necessary. Fixed in Polymer/gen-typescript-declarations#47.

justinfagnani · 2017-12-16T02:27:10Z

types/extra-types.d.ts

+
+/// <reference path="lib/mixins/property-effects.d.ts" />
+
+/**


Shouldn't this be a /* comment, since it's not a doc comment on the subsequent line?

justinfagnani · 2017-12-16T02:27:58Z

types/extra-types.d.ts

+};
+
+interface PolymerInit {
+  is: string;


Are there any comments to copy here? TODO maybe?

Added TODO. Nothing in the externs to copy so I'd be writing something new.

justinfagnani · 2017-12-16T02:29:23Z

types/extra-types.d.ts

+}
+
+/**
+ * Types from "lib/utils/async.html"


Why do we have types from regular source files? Is it cause we don't have interfaces in there? Might be worth a comment.

Some interfaces are defined directly in the source if they are only used in that one place. Not sure I need to comment that here? Did end up removing one case, since AsyncModule turned out to be a re-definition of AsyncInterface.

justinfagnani · 2017-12-16T02:37:51Z

types/lib/legacy/polymer.dom.d.ts

+     *
+     * @returns Array of assigned nodes
+     */
+    getDistributedNodes(): Array<Node|null>|null;


Some places you have Foo[] and here Array<Foo>. TODO to parse Closure and convert?

This is deliberate. If it's an array of a named type, we do Foo[], if it's anything more complicated we do Array<Foo | Bar | Baz>, following the google typescript style guide.

justinfagnani · 2017-12-16T02:38:35Z

types/lib/mixins/properties-changed.d.ts

+     * @param property Name of the property
+     * @param value Value to set
+     */
+    _setProperty(property: string, value: any): any;


should return void

Fixed by Tim in #4998

justinfagnani · 2017-12-16T02:40:57Z

types/lib/utils/html-tag.d.ts

+   *
+   * @returns Constructed HTMLTemplateElement
+   */
+  function html(strings: ITemplateArray|null, ...values: any[]): HTMLTemplateElement;


null shouldn't be allowed here

Done (updated annotation in Polymer).

justinfagnani · 2017-12-16T02:41:31Z

types/lib/utils/import-href.d.ts

+   *
+   * @returns The link element for the URL to be loaded.
+   */
+  function importHref(href: string, onload?: Function, onerror?: Function, optAsync?: boolean): HTMLLinkElement;


It'd be nice to have better types for the callbacks. Issue in Polymer core?

Yep, would just need to update the @params in import-href.html. Punting for now.

Funny, I filed an issue a ways back and it got comments recently: #4603

justinfagnani · 2017-12-16T02:54:25Z

types/extra-types.d.ts

+/**
+ * Closure type equivalence for tagged template literal function argument.
+ */
+type ITemplateArray = TemplateStringsArray;


Ah, I didn't see this when I looked at Polymer.html. Can't you just add this to the Closure -> TS type converter functions?

Fixed via Polymer/gen-typescript-declarations#47.

TimvdLippe · 2017-12-16T14:13:35Z

I have a couple more comments:

It seems that all static methods are not typed. For example https://www.polymer-project.org/2.0/docs/api/elements/Polymer.Element#staticmethod-createPropertiesForAttributes is not found in the types.
Running the tasks outputs some warnings, but I have no clue where they are coming from:

$ yarn run build generate-typescript
yarn run v1.3.2
$ gulp generate-typescript
[14:11:58] Using gulpfile ~/Projects/polymer/polymer/gulpfile.js
[14:11:58] Starting 'generate-typescript'...
Unknown syntax.
Unknown syntax.
Unknown syntax.
Could not find a name for class.
[14:12:04] Finished 'generate-typescript' after 5.85 s
Done in 6.75s.

I have opened #4998 to tighten almost all of the types to the correct value. This removes all : any, types methods that were completely missing types and fixes up some incorrect types.

aomarks · 2017-12-17T19:26:55Z

I think these would be easier to read if you define the Constructor type.

I didn't want to pollute the global namespace with this type. I would have to somehow ensure it's only defined once when multiple repos' generated types are compiled together.

aomarks · 2017-12-17T19:29:06Z

It seems that all static methods are not typed. For example https://www.polymer-project.org/2.0/docs/api/elements/Polymer.Element#staticmethod-createPropertiesForAttributes is not found in the types.

Oops, yeah quite an omission. I've added static method support in Polymer/gen-typescript-declarations#46. Turned out representing static methods for mixins required significant refactoring of the way mixins are emitted.

aomarks · 2017-12-17T19:32:07Z

One thing that would be nice is if we could run the generated types through the type checker to make sure they're at least well formed and all referenced built-in types are defined

I've been testing manually, the current typings compile cleanly. It would be nice to automate it, maybe fail on Travis if it's not compiling. Maybe we should do the same for Closure compile too. Don't think I'll do that in this PR though.

googlebot · 2017-12-18T22:03:15Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

aomarks · 2017-12-19T00:52:47Z

Running the tasks outputs some warnings, but I have no clue where they are coming from:

I would like to improve the warnings output by integrating with the very nice Analyzer warnings printer. For another time though.

Unknown syntax.
Unknown syntax.
Unknown syntax.

These were from not supporting Object<foo, bar> syntax. Fixed in Polymer/gen-typescript-declarations#48.

Could not find a name for class.

This is from https://github.com/Polymer/polymer/blob/master/lib/utils/templatize.html#L61. Safe to ignore.

aomarks · 2017-12-19T00:55:32Z

I think this is ready to merge. All comments addressed.

justinfagnani · 2017-12-19T04:16:24Z

LGTM still

TimvdLippe

This LGTM for now. There are still some incorrect : any and incorrectly exposed private variables, but these are very few nits. I think it is better to merge this now and let end-users play with TypeScript support to get feedback, rather than perfect it right away. Nice work @aomarks !

43081j · 2017-12-19T16:17:25Z

Just a couple of things but they could be sorted in a future PR:

Annotations:

dom-repeat's indexForElement has an annotation of {*} for its return type, but should probably be number
properties-changed mixin should have createdProperties as void return type
properties-mixin should have finalize as void return type
async animationFrame.run should have function(number):void parameter
render-status beforeNextRender and afterNextRender parameters should be function():void

If you want to get this merged, i don't mind doing a PR for these annotations.

Misc:

does anyone know of progress or a PR to add connectedCallback and what not to typescript's shipped definitions? there's a few places we have to define these but they should really be inherited
animationFrame.run and animationFrame.cancel are duplicated

Great work @aomarks, will be testing this out more through the week.

aomarks requested review from dfreedm and justinfagnani November 10, 2017 20:27

googlebot added the cla: yes label Nov 10, 2017

dfreedm reviewed Nov 14, 2017

View reviewed changes

TimvdLippe mentioned this pull request Nov 16, 2017

WIP: First attempt at generated TypeScript declarations. Not there yet. #4377

Closed

aomarks force-pushed the aomarks-typescript branch 5 times, most recently from 28a8603 to 9c00f7e Compare November 29, 2017 22:41

tpluscode mentioned this pull request Nov 30, 2017

HTMLElement life-cycle callbacks Buslowicz/twc#121

Closed

TimvdLippe assigned aomarks Dec 3, 2017

aomarks force-pushed the aomarks-typescript branch from 86d7a99 to 23fe44a Compare December 8, 2017 23:27

Annotate klass class as @Private.

b02c458

Annotate that dedupingMixin returns T.

aomarks force-pushed the aomarks-typescript branch from 23fe44a to 1ef2cec Compare December 8, 2017 23:28

aomarks added 2 commits December 8, 2017 15:29

Add script to generate TypeScript declarations.

1f9be78

Includes hand written types for cases that couldn't be generated. Uses https://github.com/Polymer/gen-typescript-declarations. Run with `gulp generate-typescript`. Also asyncify the gulpfile with fs-extra.

Add generated TypeScript declarations.

60450bf

aomarks force-pushed the aomarks-typescript branch from 1ef2cec to 60450bf Compare December 8, 2017 23:29

dfreedm requested changes Dec 8, 2017

View reviewed changes

aomarks added 2 commits December 8, 2017 19:15

Update CHANGELOG.

3ffb895

Fix html-tag import path.

c9be530

TimvdLippe suggested changes Dec 10, 2017

View reviewed changes

This was referenced Dec 11, 2017

void type is not nullable. Polymer/gen-typescript-declarations#40

Merged

Constructors, records, void returns, method descs. Polymer/gen-typescript-declarations#41

Merged

Bring in latest gen-typescript-declarations updates.

6a0d214

Mark some FlattenedNodesObserver things private.

5190a89

aomarks mentioned this pull request Dec 15, 2017

Tighten Closure type annotations. #4997

Merged

aomarks added 3 commits December 15, 2017 15:57

Merge branch 'master' into aomarks-typescript

660d64f

Bump gen-typescript version.

266d599

New typings.

a1f3317

justinfagnani approved these changes Dec 16, 2017

View reviewed changes

TimvdLippe mentioned this pull request Dec 16, 2017

Tighten more types for TypeScript and Closure #4998

Merged

aomarks and others added 2 commits December 17, 2017 15:57

Add renameTypes config.

73666c3

Tighten more types for TypeScript and Closure (#4998)

e872982

googlebot added cla: no and removed cla: yes labels Dec 18, 2017

aomarks added 2 commits December 18, 2017 14:24

Update PropertyEffects interface name in remap config.

0ebfc24

Address latest round of comments.

7b581de

TimvdLippe approved these changes Dec 19, 2017

View reviewed changes

dfreedm approved these changes Dec 19, 2017

View reviewed changes

aomarks merged commit ee1d71f into master Dec 19, 2017

43081j mentioned this pull request Dec 19, 2017

add void return type annotations #5000

Merged

dfreedm deleted the aomarks-typescript branch December 19, 2017 23:29

TimvdLippe mentioned this pull request Dec 21, 2017

Fail travis on non updated files #5003

Merged

FredKSchott mentioned this pull request Dec 21, 2017

Fix settings so that its properly picked up by both gen-ts and modulizer #5004

Merged


		/// <reference path="lib/mixins/property-effects.d.ts" />

		/**

Add TypeScript declarations. #4928

Add TypeScript declarations. #4928

Conversation

aomarks commented Nov 10, 2017 • edited Loading

dfreedm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani commented Nov 14, 2017

aomarks commented Nov 17, 2017 • edited Loading

TimvdLippe commented Nov 18, 2017

aomarks commented Dec 8, 2017

aomarks commented Dec 8, 2017

dfreedm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aomarks Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimvdLippe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aomarks Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aomarks commented Dec 16, 2017

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimvdLippe commented Dec 16, 2017

aomarks commented Dec 17, 2017

aomarks commented Dec 17, 2017

aomarks commented Dec 17, 2017

googlebot commented Dec 18, 2017

aomarks commented Dec 19, 2017 • edited Loading

aomarks commented Dec 19, 2017

justinfagnani commented Dec 19, 2017

TimvdLippe left a comment

Choose a reason for hiding this comment

43081j commented Dec 19, 2017 • edited Loading

aomarks commented Nov 10, 2017 •

edited

Loading

aomarks commented Nov 17, 2017 •

edited

Loading

aomarks Dec 9, 2017 •

edited

Loading

aomarks Dec 12, 2017 •

edited

Loading

aomarks commented Dec 19, 2017 •

edited

Loading

43081j commented Dec 19, 2017 •

edited

Loading