Skip to content

Add support for CORS in WebMessageHandler#106

Open
lkraider wants to merge 21 commits into
j2labs:masterfrom
nKey:cors
Open

Add support for CORS in WebMessageHandler#106
lkraider wants to merge 21 commits into
j2labs:masterfrom
nKey:cors

Conversation

@lkraider

Copy link
Copy Markdown
Collaborator

Allow easy support for Cross-Origin Resource Sharing requests by
simply returning a list of hosts on the cors_allow_origin method.

@gone

gone commented Feb 19, 2013

Copy link
Copy Markdown
Collaborator

Dude awesome job!

I really like the idea of using prepare to determine if the resource is CORS-able or not - but can you split that out into a separate decorator? My understanding is that prepare is really meant for subclasses to overwrite - I don't want to depend on functionality there. Also making it into a decorator allows people to explicitly mark which resources are for cors and which ones are not.

Can you make origin, headers, methods, exposed headers, and allowed credentials properties, rather than methods? Keeping those static as much as possible should make things faster.

What do you think about turning the part that handles credentials/allow-origin = * into a function so it's not duplicated? I could go either way on that one.

Thoughts?

@lkraider

Copy link
Copy Markdown
Collaborator Author

Thanks for reviewing it!

I was already pending towards defining it as a decorator, makes sense.

Turning those methods into simple instance attributes is better. I think I'll override __init__ for that then, as initialize is called on clear_payload and it's unnecessary to reset those values there. Although subclasses will probably redefine them there.

I thought about turning that snippet into a function, but since on runtime it's basically a condition check and an assignment, I thought a function call would be more overhead than repeating a little code.

@gone

gone commented Feb 19, 2013

Copy link
Copy Markdown
Collaborator

that's probably true that a function is more overhead.

I think since they're static they should be class properties, rather than
using init. If subclasses need dynamic attributes, they can use
@Property

I think that will be faster/less overhead, and semantically I think it's
better - resources generally don't have different CORS rules depending on
the request.

On Tue, Feb 19, 2013 at 4:38 PM, Paul Eipper notifications@github.comwrote:

Thanks for reviewing it!

I was already pending towards defining it as a decorator, makes sense.

Turning those methods into simple instance attributes is better. I think
I'll override init for that then, as initialize is called on
clear_payload and it's unnecessary to reset those values there. Although
subclasses will probably redefine them there.

I thought about turning that snippet into a function, but since on runtime
it's basically a condition check and an assignment, I thought a function
call would be more overhead than repeating a little code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/106#issuecomment-13801306.

@gone

gone commented Feb 19, 2013

Copy link
Copy Markdown
Collaborator

Also this is so, so slick. I have a bunch of crappy patched @cors_support
resources in an app that just set allowed headers -I'd love to replace
those with something better designed.

On Tue, Feb 19, 2013 at 4:50 PM, Ben Beecher benbeecher@gmail.com wrote:

that's probably true that a function is more overhead.

I think since they're static they should be class properties, rather than
using init. If subclasses need dynamic attributes, they can use
@Property

I think that will be faster/less overhead, and semantically I think it's
better - resources generally don't have different CORS rules depending on
the request.

On Tue, Feb 19, 2013 at 4:38 PM, Paul Eipper notifications@github.comwrote:

Thanks for reviewing it!

I was already pending towards defining it as a decorator, makes sense.

Turning those methods into simple instance attributes is better. I think
I'll override init for that then, as initialize is called on
clear_payload and it's unnecessary to reset those values there. Although
subclasses will probably redefine them there.

I thought about turning that snippet into a function, but since on
runtime it's basically a condition check and an assignment, I thought a
function call would be more overhead than repeating a little code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/106#issuecomment-13801306.

@lkraider

lkraider commented Mar 8, 2013

Copy link
Copy Markdown
Collaborator Author

I believe I addressed your points with these commits, although I didn't test the code thoroughly after the refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants