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

Tighten Closure type annotations. #4997

Merged
merged 2 commits into from
Dec 15, 2017
Merged

Tighten Closure type annotations. #4997

merged 2 commits into from
Dec 15, 2017

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Dec 15, 2017

I pulled these changes out of #4928. These are various changes to the Polymer core JSDoc annotations that improve the generated TypeScript declarations.

  • Adds various ! non-nullable operators. In Closure, all non-primitive types are non-nullable (we should try to use non-nullable types by default going forward if possible).
  • Marked some things @private that are not part of our public API.
  • Added @return {void} to some functions that don't return anything.
  • A few other signature changes.

gulp closure produces no warnings

@@ -211,7 +211,7 @@

/**
* Clears the selection state.
*
* @returns {void}
Copy link
Member

Choose a reason for hiding this comment

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

@return

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -61,16 +61,19 @@
this.__children = null;
}

// assumes only one observed attribute
/** @returns {void} */
Copy link
Member

Choose a reason for hiding this comment

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

@return

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

this.mutableData = true;
}

/** @returns {void} */
Copy link
Member

Choose a reason for hiding this comment

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

@return is only necessary if the function takes arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

See below. Think we should keep these.

connectedCallback() {
this.style.display = 'none';
this.render();
}

/** @returns {void} */
Copy link
Member

Choose a reason for hiding this comment

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

@return is only necessary if the function takes arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it because otherwise in TypeScript we have to output returns any in this case, because we have no idea what the return type is.

@@ -74,6 +74,10 @@
return null;
}

/**
* @param {string} name
* @returns {void}
Copy link
Member

Choose a reason for hiding this comment

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

@return

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -74,6 +74,10 @@
return null;
}

/**
* @param {string} name
Copy link
Member

@dfreedm dfreedm Dec 15, 2017

Choose a reason for hiding this comment

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

Add
@param {?string} old
@param {?string} value

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Also had to add descriptions according to linter.

@@ -134,6 +134,7 @@
* @param {string} name Name of attribute.
* @param {?string} old Old value of attribute.
* @param {?string} value Current value of attribute.
* @returns {void}
Copy link
Member

Choose a reason for hiding this comment

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

@return

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@dfreedm dfreedm left a comment

Choose a reason for hiding this comment

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

LGTM

@aomarks aomarks merged commit ee4445f into master Dec 15, 2017
@aomarks aomarks deleted the aomarks-annotations branch December 15, 2017 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants