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

Convert .ts to .d.ts files #61

Merged
merged 1 commit into from Nov 3, 2020
Merged

Conversation

mrdrogdrog
Copy link
Contributor

@mrdrogdrog mrdrogdrog commented Oct 27, 2020

Good evening,

Some days ago I found out, that the picker types don't have a signature for removeEventListener, so I was about to create an issue. But then I though "Why don't you fix that?". Then I saw your types and the dummy classes and five shaved yaks later I'm here. (Yes I read the contribution guidelines.)

This PR:

  • Converts all .ts files to .d.ts files and removes the dummy implementations.
  • Gets rid of @ts-ignore
  • Changes the typedoc call to also include the .d.ts files
  • Changes the generateTOC script, so it still finds the scripts
  • Add signatures for removeEventListener

Feel free to review!

@mrdrogdrog mrdrogdrog changed the title Convert types to .d.ts files Convert .ts to .d.ts files Oct 27, 2020
@nolanlawson
Copy link
Owner

This is a great change, thank you. I'm not a super-duper TypeScript expert, so that's why I did things the way I did. 🙂

Nonetheless, I actually did realize recently that I could still use typedoc with just .d.ts files and --includeDeclarations, so I was considering making this change myself. But you saved me the trouble. So thanks a ton! ❤️

package.json Outdated
"build:typedoc": "typedoc --target ES5 --out docs-tmp --theme markdown --excludePrivate --excludeNotExported --hideSources --hideBreadcrumbs ./src/types && node ./bin/generateTypeDocs && rm -fr docs-tmp",
"build:types": "tsc --target ES5 -d --outDir ./ts-tmp ./src/types/*.ts && mv ./ts-tmp/*.d.ts ./ && rm -fr ts-tmp",
"build:typedoc": "typedoc --target ES5 --out docs-tmp --theme markdown --excludePrivate --excludeNotExported --hideSources --includeDeclarations --hideBreadcrumbs ./src/types && node ./bin/generateTypeDocs && rm -fr docs-tmp",
"build:types": "cp ./src/types/*.d.ts ./",
Copy link
Owner

Choose a reason for hiding this comment

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

We might as well just put the .d.ts files at the top level, but I can change that later

Copy link
Owner

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Unfortunately it looks like this messes up the README a bit due to how typedoc works. E.g. some methods that are supposed to return void now return any, and the picker options and defaults are all gone... I wonder if there is still a way to fix this with just .d.ts files, or if it's a limitation of typedoc and therefore we should stick to dummy TS files. Another tricky part is that typedoc is out-of-date, and upgrading results in a messed-up README file... this may need some additional work 😕

(yarn build will rebuild the README file btw)

@nolanlawson
Copy link
Owner

^ Short-term fix for the removeEventListener issue

@mrdrogdrog
Copy link
Contributor Author

I think I can solve the remaining issues :)

Please stay tuned

@mrdrogdrog
Copy link
Contributor Author

The readme looked fine when I tested the PR 😅
But I'll dig deeper and try to solve the issues :)

@mrdrogdrog
Copy link
Contributor Author

Okay I did some researches:

  • .d.ts files can't handle default values. You could put them into the docs, but that is kinda ugly.
  • The only any type I found was at the setter. But that's okay

But I could still fix the @ts-ignore stuff.

@mrdrogdrog
Copy link
Contributor Author

BTW: @ts-ignore was needed, because you didn't add a implementation for addEventListener. Only the signature.

@nolanlawson
Copy link
Owner

Ah, that's too bad. Thanks for doing the research, though! Looks like we should probably stick with the .ts files for now then. I appreciate the fix!

@nolanlawson nolanlawson merged commit f6b3d83 into nolanlawson:master Nov 3, 2020
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.

None yet

2 participants