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

Keep headers after an error #11

Merged
merged 1 commit into from
Apr 26, 2016
Merged

Conversation

PlasmaPower
Copy link
Contributor

If this is merged I'll rewrite it for v2.x.

@PlasmaPower
Copy link
Contributor Author

Oops, forgot we have to test for node 0.12, where Object.assign doesn't exist. Fixing now.

@fengmk2
Copy link
Member

fengmk2 commented Mar 17, 2016

This feature should be another middleware I think.

@fengmk2
Copy link
Member

fengmk2 commented Mar 17, 2016

Ok, I see this just want to set cors headers only on error. This should be ok.

And now we should think about performance. Don't bind set function every request. set(this, key, value) will be better. And don't try catch if show header option is false.

@PlasmaPower
Copy link
Contributor Author

Is this good? The set function still has to be local because it needs a local copy of headersSet.

@@ -49,6 +50,8 @@ module.exports = function (options) {

options.credentials = !!options.credentials;

options.headersKeptOnError = options.headersKeptOnError === undefined || !!options.headersKeptOnError;
Copy link
Member

Choose a reason for hiding this comment

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

should be keepHeadersOnError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this was from when I was planning on making it an array so you could only keep some. Will be fixed in just a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

} else {
// Preflight Request

// If there is no Access-Control-Request-Method header or if parsing failed,
// do not set any additional headers and terminate this set of steps.
// The request is outside the scope of this specification.
if (!this.get('Access-Control-Request-Method')) {
Copy link
Member

Choose a reason for hiding this comment

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

on Preflight Request, there is won't throw any error on old code logic https://github.com/koajs/cors/blob/master/index.js#L88.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah I guess it won't have set any headers. Reverting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, that should simplify the diff quite a bit.

@fengmk2
Copy link
Member

fengmk2 commented Apr 26, 2016

+1

@fengmk2 fengmk2 merged commit 677833d into koajs:master Apr 26, 2016
@fengmk2
Copy link
Member

fengmk2 commented Apr 26, 2016

1.2.0 release.

@PlasmaPower please pick this feature into v2.x branch

PlasmaPower added a commit to PlasmaPower/cors that referenced this pull request Apr 26, 2016
PlasmaPower added a commit to PlasmaPower/cors that referenced this pull request Apr 28, 2016
PlasmaPower added a commit to PlasmaPower/cors that referenced this pull request Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants