Skip to content

Add types/jsonwebtokens-promisified#25038

Merged
RyanCavanaugh merged 3 commits intoDefinitelyTyped:masterfrom
aneilbaboo:aneil/jsonwebtoken-promisified
Apr 17, 2018
Merged

Add types/jsonwebtokens-promisified#25038
RyanCavanaugh merged 3 commits intoDefinitelyTyped:masterfrom
aneilbaboo:aneil/jsonwebtoken-promisified

Conversation

@aneilbaboo
Copy link
Copy Markdown
Contributor

Overview

The jsonwebtoken-promisified npm module adds a couple of methods: verifySync and decodeSync. This new declaration covers the methods of jsonwebtoken and adds these extra promise-returning methods.

Checklist

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

@typescript-bot typescript-bot added New Definition This PR creates a new definition package. Awaiting reviewer feedback labels Apr 16, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 16, 2018

@aneilbaboo Thank you for submitting this PR!

Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes.

In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day!

/types/jsontoxml/ @benstevens48
/types/jsonwebtoken/ @SomaticIT @danielheim @brikou
/types/jsonwebtoken-promisified @aneilbaboo
/types/jspdf/ @amberjs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a problem but FYI you don't need to modify this file yourself - it is supposed to be auto-generated

@@ -0,0 +1,79 @@
{
"extends": "dtslint/dt.json",
"rules": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I need some justification for why you've disabled all the lint rules. We have them all in place for a reason.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. I copied the tsconfig from types/jsonwebtoken.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. The linting caught a lot of stupid stuff. I'm fixing it.

I looked at some of the nearby folders, and noticed ~50% of them have the same tsconfig. I suspect DefinitelyTyped is littered with this bad pattern.

E.g,: jsonnet, jsonpath jsonstream, jspdf, jsrender

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what's happening. The javascript has function signatures that aren't easily matched by typescript rules. I believe the underlying library must be doing runtime inspection of arguments. So enabling the linting rules creates a bunch of errors.

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Apr 16, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

@aneilbaboo One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@aneilbaboo
Copy link
Copy Markdown
Contributor Author

@RyanCavanaugh, Removed the tslint ignores and resulting issues

@typescript-bot typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Apr 17, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

🔔 @RyanCavanaugh - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@RyanCavanaugh RyanCavanaugh merged commit 33babb4 into DefinitelyTyped:master Apr 17, 2018
@aneilbaboo aneilbaboo deleted the aneil/jsonwebtoken-promisified branch April 25, 2018 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Definition This PR creates a new definition package.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants