mreinstein / alexa-verifier

✓ Verify HTTP requests sent to an Alexa skill are sent from Amazon
MIT License
76 stars 23 forks source link

Support for promises (and wrapper included in comment) #18

Closed geuis closed 7 years ago

geuis commented 7 years ago

Would be great for this to natively support promises. After looking through the code it wouldn't take much work to add support for it. In the meantime, if you would like to use this library as a promise, you can use the following wrapper.

const verifier = require('alexa-verifier');

const verifyRequest = (signatureCertUrl, signature, body) => {
  return new Promise((resolve, reject) => {
    verifier(signatureCertUrl, signature, body, (err) => {
      if (err) {
        return reject(err);
      };

      return resolve(body);
    });
  });
};

verifyRequest(req.headers.signaturecertchainurl, req.headers.signature, body)
  .then((body) => console.log('Success', body))
  .catch((err) => console.log('Error', err));
dblock commented 7 years ago

You should contribute. Would this make integration into alexa-app/alexa-app-server simpler?

mreinstein commented 7 years ago

@geuis better to not use native promises yet. bluebird offers significant performance improvements, given the current state of v8. after turbofan lands in v8 5.7 this may not be true anymore, but right now it definitely is much slower.

Besides better performance than current native promises, bluebird also results in less userland code to write:

const Promise = require('bluebird')

const verifyRequest = Promise.promisify(require('alexa-verifier'))
mreinstein commented 7 years ago

@geuis, are you basing your alexa skill on express? if so, you might be better off using this middleware, which uses alexa-verifier under the hood: https://www.npmjs.com/package/alexa-verifier-middleware

geuis commented 7 years ago

@mreinstein As long as the bluebird interface is concurrent with the Promise spec, I'd say that would be fine that's the route you want to go. However, I tend to fall on the side of the line to use native language features where possible. Only IE11 doesn't currently support the Promise spec in es2015 as far as I know, but I don't believe your library is intended for use in browser environments (unless mistaken, of course.) Wrapping your library in a bluebird promise certainly is simple, but then you're introducing another dependency to an otherwise lovely and light-weight utility.

In my case, I'm writing my own skill app without using Express. Had originally found the alexa-js fork of this library that they've adapted as Express middleware, then from there found your project.

The sample code I left above is just an FYI for any other users who come across this and want to easily use alexa-verifier with promises without you needing to do any changes on your project.

Thanks for your work on this. Its been super helpful.

geuis commented 7 years ago

@dblock I'm writing my skill server from scratch (not a huge fan of Express) and when searching for a verifier utility I came across this one. Before I would consider doing a pull request to add promise support for alexa-verifier, I wanted to post this to get feedback from the project owners first. No point in doing work that no one wants. =)

mreinstein commented 7 years ago

I'm writing my skill server from scratch (not a huge fan of Express)

@geuis much props. I like express, but I think people are way too eager to just use a framework and follow the herd without thinking. I don't use alexa-app or alexa-app-server much for the same reasons. Cargo cult programming is poison.

geuis commented 7 years ago

@mreinstein Totally agreed! I'm enjoying the process of reading through the docs https://developer.amazon.com/public/solutions/alexa/alexa-skills-kit/docs/handling-requests-sent-by-alexa and building it up piece by piece. This way I can understand what's actually happening at a deeper level. At a certain point I may make the choice to just switch over and use a library for convenience, but usually not until I at least have a basic service running.

mreinstein commented 7 years ago

I tend to fall on the side of the line to use native language features where possible

Me too. Because of this, I was very surprised that after switching from callback hell to native promises, I was seeing a 4-6X slowdown in my application's throughput. Digging in I discovered this

http://softwareengineering.stackexchange.com/questions/278778/why-are-native-es6-promises-slower-and-more-memory-intensive-than-bluebird

which give a pretty good overview of what was happening.

Wrapping your library in a bluebird promise certainly is simple, but then you're introducing another dependency to an otherwise lovely and light-weight utility.

I agree, but given the performance impacts I was seeing with real life production code, I couldn't tradeoff elegance for a 400-600% slowdown of some parts of my codebase. Switching to bluebird at least made this about a 200% slowdown which was more reasonable for my use case.

that said, turbofan is looking pretty exciting, and should have a dramatic impact on promise and general es6 performance. It's scheduled to land in chrome 59 (which will likely end up in node 8) At that point this would be worth re-evaluating. I would love to be in the spot where bluebird could be dropped in favor of just using the built-in promises.