shipharbor / merry

:ocean::ocean::sailboat::ocean::ocean: - cute streaming API framework
MIT License
312 stars 21 forks source link

fix cors stuff to set headers on res #64

Closed lrlna closed 7 years ago

lrlna commented 7 years ago

Reworking our cors stuff! This sets up a some default options so folks don't have to.

Returns a handler, instead of an object with response headers set on the response :sparkles:.

Let me know whatchu think ^__^

closes #60

yoshuawuyts commented 7 years ago

Quick question: what happens to OPTIONS preflight headers in this case? Are we handling those? - is this doing what it should rn?

lrlna commented 7 years ago

@yoshuawuyts unless i am misunderstanding how OPTIONS work, I think that should just be able to be passed in with merry.cors({methods: 'OPTIONS'}) and then just be assigned to res; was reading through spec and it doesn't look like it's suppose to be something special. Am I missing something there?

EDIT: ^^ the code thingy was missing ticks and didn't look good

lrlna commented 7 years ago

SORRY I AM CHANGING MY MINDDDD ON THIS but I swear I think this is gonna be better!

Instead of adding stuff to opts.methods, I think we should just reset it. SO! If a human doesn't want the entirety of what comes as default, e.g. all the headers and methods,

  var headers = ['Content-Type', 'Accept', 'X-Requested-With']
  var methods = ['PUT', 'POST', 'DELETE', 'GET', 'OPTIONS']

they should pass those in opts and we will just overwrite the default ones. Having all of those methods is too permissive, so not everyone might want that.

yay? nay?

yoshuawuyts commented 7 years ago

We should also probs link to https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS

lrlna commented 7 years ago

which From?

On Sat, Feb 11, 2017, 12:44 Yoshua Wuyts notifications@github.com wrote:

We should also probs link to From

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/merry/pull/64#issuecomment-279141610, or mute the thread https://github.com/notifications/unsubscribe-auth/AHu3CNT9dbpy6FdJcP1NRp7GrlPKTL_Xks5rba0QgaJpZM4L3eSP .

yoshuawuyts commented 7 years ago

typo; open up the github issue ya dingus

lrlna commented 7 years ago

Wowzaaa stop yellin' geeeeez :p

But yea okay I'll link up to that.

On Sat, Feb 11, 2017, 12:48 Yoshua Wuyts notifications@github.com wrote:

typo; open up the github issue ya dingus

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/merry/pull/64#issuecomment-279141832, or mute the thread https://github.com/notifications/unsubscribe-auth/AHu3CNZ2zjE9XmN4rkLn1T38SrEMafozks5rba4ogaJpZM4L3eSP .

yoshuawuyts commented 7 years ago

how about... we make this one a middleware?

codecov-io commented 7 years ago

Codecov Report

Merging #64 into master will increase coverage by 7.74%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   81.42%   89.17%   +7.74%     
==========================================
  Files           5        5              
  Lines         210      194      -16     
==========================================
+ Hits          171      173       +2     
+ Misses         39       21      -18
Impacted Files Coverage Δ
index.js 82.52% <ø> (+11.79%) :white_check_mark:
middleware.js 100% <ø> (ø) :white_check_mark:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b628b06...452a7a8. Read the comment docs.

lrlna commented 7 years ago

Okay, I've abstracted this out to a middleware plugin -- cors-middleware and could be applied as part of middleware, this PR now just has the docs mentioning how people can get it to work.

Since we are technically removing this from merry (even though it wasn't working before), this is a ✨ breaking change ✨.

yoshuawuyts commented 7 years ago

Woooooot

On Wed, Mar 1, 2017, 16:59 Irina Shestak notifications@github.com wrote:

Okay, I've abstracted this out to a middleware plugin -- cors-middleware https://github.com/shipharbor/cors-middleware and could be applied as part of middleware, this PR now just has the docs mentioning how people can get it to work.

Since we are technically removing this from merry (even though it wasn't working before), this is a ✨ breaking change ✨.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/shipharbor/merry/pull/64#issuecomment-283381565, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlesl0ejkjg_YNikgFDFFB-N5Z5gq-ks5rhZXlgaJpZM4L3eSP .

lrlna commented 7 years ago

cors is no longer explicitly handled in merry :sparkles: