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.

Sign up for GitHub

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

Jump to bottom

Add TypeScript declarations to marked-element #88

Merged
merged 5 commits into from
Mar 1, 2018
Merged

Add TypeScript declarations to marked-element #88

merged 5 commits into from
Mar 1, 2018

Conversation

stramel
Copy link
Collaborator

@stramel stramel commented Feb 28, 2018 •
edited
Loading

This PR adds TypeScript declarations generated by https://github.com/Polymer/gen-typescript-declarations/

These declarations can be re-generated by running npm run update-types.

Tracker: https://github.com/Polymer/gen-typescript-declarations/issues/79

Noticed this one wasn't listed on the tracking issue. Figured I would get it started.

Keep running out of memory on my work PC, will try generating when I get home.

@stramel stramel changed the title Generate type declarations Add TypeScript declarations to marked-element Feb 28, 2018
@aomarks
Copy link
Contributor

aomarks commented Feb 28, 2018 •
edited
Loading

Thanks for the PR! I don't see the actual typings in the commit, do you need to git add them?

When I tried this element earlier, it wasn't compiling. One problem (hopefully the only one) was that there is no ../marked/lib/marked.d.ts (meaning in the marked/ bower component). I think we probably don't need typings from marked itself, so you can just use the removeReferences config option to prune out that <reference> (example here: https://github.com/Polymer/polymer/blob/master/gen-tsd.json#L10).

To test if things compile, you need to tsc --init, bower install, shuffle the files around so that the bower components are in the right place (as siblings), and then tsc. One easy way to shuffle the files around is mv bower_components/* ../ (but you probably want an extra directory layer so that you don't get any collisions with whatever was already in your parent directory).

FYI all of the elements that don't currently have a PR out for them are because they aren't yet compiling. They probably all just need a few little tweaks like this (or depend on ones that do), and I haven't quite had the time yet to go through them. paper-behaviors (already has a PR, but not compiling so I haven't merged yet -- PolymerElements/paper-behaviors#85) and paper-input would be the ideal next ones to tackle if you're interested in helping out, because they have the most dependencies.

@stramel
Copy link
Collaborator Author

stramel commented Mar 1, 2018

@aomarks Thank you so much for the help in how to test it. It seems like it is compiling now with these changes.

I did notice that the tool doesn't run on windows. I will file an issue.

I went ahead and added just some basic @params in there to help the typings. I can revert that if preferred.

Without the ignore of demo/** the tool tried to generate a type declaration for demo files as well. I'm not sure how you got around that on other repositories.

@aomarks
Copy link
Contributor

aomarks commented Mar 1, 2018 •
edited by stramel
Loading

I did notice that the tool doesn't run on windows. I will file an issue.

Sounds good.

I went ahead and added just some basic @params in there to help the typings. I can revert that if preferred.

Great.

Without the ignore of demo/** the tool tried to generate a type declaration for demo files as well. I'm not sure how you got around that on other repositories.

Replied on the commit.

aomarks
aomarks reviewed Mar 1, 2018
View reviewed changes
gen-tsd.json Outdated
@@ -0,0 +1,10 @@
{
"excludeFiles": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default set of excluded files, so you can omit it (https://github.com/Polymer/gen-typescript-declarations/blob/master/src/gen-ts.ts#L66)

.travis.yml Outdated
install:
- npm install -g polymer-cli
- npm install -g polymer-cli bower
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be neccessary -- bower is a devDependency from the package.json.

marked-element.html Outdated
@@ -372,6 +380,7 @@
/**
* Fired when an error is received while fetching remote markdown content.
*
* @param {Event} e
Copy link
Contributor

Choose a reason for hiding this comment

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

!Event (object types in Closure are nullable by default)

marked-element.html Outdated
@@ -381,6 +390,9 @@
}
},

/**
* @param {MutationRecord} mutation
Copy link
Contributor

Choose a reason for hiding this comment

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

I see mutation[0] right below, so maybe this is supposed to be an array? In which case (again because of default nullability) it should be !Array<!MutationRecord>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shoot! Yes, I mean to do that!

aomarks
aomarks reviewed Mar 1, 2018
View reviewed changes
marked-element.html Outdated
@@ -253,7 +253,8 @@
/**
* Unindents the markdown source that will be rendered.
*
* @param {string} text
* @param {!string} text
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't actually need it for primitives like string, they are already non-null by default. :)

https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#nullable-type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL 👍

aomarks
aomarks reviewed Mar 1, 2018
View reviewed changes
.travis.yml
install:
- npm install -g polymer-cli
- npm install
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this? Travis automatically runs it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't automatically run this when it is in the install section

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Our Travis files seem inconsistent, some of the other ones don't use the install section at all.

https://github.com/PolymerElements/iron-behaviors/blob/master/.travis.yml

Oh well, fine.

@aomarks aomarks merged commit 9f4160b into PolymerElements:master Mar 1, 2018
@stramel stramel deleted the gen-typescript-declarations branch March 1, 2018 05:02
@aomarks
Copy link
Contributor

aomarks commented Mar 1, 2018

Released as v2.4.0 -- thanks again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aomarks aomarks aomarks left review comments

@e111077 e111077 Awaiting requested review from e111077 e111077 is a code owner

@dfreedm dfreedm Awaiting requested review from dfreedm

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stramel @aomarks @googlebot