alexa-js / alexa-verifier-middleware

An express middleware that verifies HTTP requests sent to an Alexa skill are sent from Amazon.
MIT License
31 stars 6 forks source link

Typescript definitions #46

Closed evandrouzeda closed 2 years ago

evandrouzeda commented 2 years ago

Just adding a typescript definition for typescript users

dblock commented 2 years ago

Add a line to https://github.com/alexa-js/alexa-verifier-middleware/blob/master/CHANGELOG.md and let's bump the version to 2.1.0 in the same PR for semver?

tejashah88 commented 2 years ago

@dblock @mreinstein I wonder if it's worth porting this project to typescript directly. I'm not quite up to speed on the TS community so I want to hear your thoughts.

mreinstein commented 2 years ago

unpopular opinion: I don't care about typescript

dblock commented 2 years ago

I haven't used this project in a while so I don't have any strong opinion. IMHO TypeScript definitions are must have today because most people write TypeScript and not raw javascript to keep their sanity :)

mreinstein commented 2 years ago

Just for the heck of it I looked through all issues that this repo has ever opened. All 23 of them. Of those, 0 would have been prevented by including typescript in this project. In exchange for introducing a compile toolchain many many thousands of lines long. That sounds like real insanity to me.

This is exactly the kind of modern decision making that is contributing to the overall decline in software quality. Chasing local optimums and introducing massive project complexity to avoid simple bugs that even average programmers can avoid with ease.

Jonathan Blow had it exactly right https://www.youtube.com/watch?v=ZSRHeXYDLko

tejashah88 commented 2 years ago

Coming back to this repository, the suggested type definition is pretty useless given the only type used is any. Even if we look at how morgan and body-parser implemented their types, it seems like a lot more maintenance for an ExpressJS middleware package that's really easy to use. (e.g app.use(verifier)).

My (rudimentary) understanding is that adding typing information in a JS library makes sense when the project is large enough where not having said types can cause future headaches for developers. While I appreciate the contribution, (for now) I don't see any significant benefit from adding TypeScript support as this library is really simple to use.

tejashah88 commented 2 years ago

I'd argue that it's at least more worth porting the alexa-app repository and such bigger projects to TS, but those repos have not been well maintained and there's a lot of open issues to resolve (another discussion for another time).

mreinstein commented 2 years ago

I people want typescript definitions in this repo, fine. It seems harmless as it just sits there as an inert file for people not using typescript.

I absolutely do not want to introduce a compile step into this repo or otherwise force everyone into using typescript.

dblock commented 2 years ago

Coming back to this repository, the suggested type definition is pretty useless given the only type used is any.

This should be fixed in this PR I think. @evandrouzeda looks like @mreinstein would be ok with the type definition, can we have something other than any in this PR?

The types of the objects passed isn't any, there's a request and the code accesses fields such as body or rawBody which come from somewhere. Even for such a rudimentary project knowing reliably that those actually exist, and are of the correct type, could be useful.

dblock commented 2 years ago

I absolutely do not want to introduce a compile step into this repo or otherwise force everyone into using typescript.

The compile step argument is a valid one.

evandrouzeda commented 2 years ago

I added more details to definitions