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

bundle/cors: introduction #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

achiang
Copy link

@achiang achiang commented Feb 2, 2019

Add a new bundle to provide CORS support.

Note: let me know what else you'd like to see before accepting this PR for merge, such as tests (or other).

Also: regarding this bundle's config, I haven't actually tested that overriding the settings will propagate all the way into flask-cors; it's just based on my reading of the flask-cors code, and my sorta vague understanding of bundle initialization. I linked to the code in flask-cors in my commit log that makes me think it will Just Work.

We'll surely want to reconfigure whether we accept cross-origin cookies
at some point, but let's just try this for now.
flask-cors supports app-level config already:

  https://github.com/corydolphin/flask-cors/blob/9f56fd79b081c19876ccdabdf217e9f36260c4a4/flask_cors/core.py#L298

So, all we do here is expose the same config vars in our bundle, where
the default values can be overridden, and then picked up automatically
by flask-cors.
class Config(BundleConfig):
# Default values; override as necessary
CORS_ORIGINS = '*'
CORS_METHODS = ALL_METHODS
Copy link
Owner

Choose a reason for hiding this comment

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

Upstream's default is already all methods so I think we can leave this out (the ALL_METHODS constant from flask_unchained is perhaps confusingly named, it doesn't include HEAD)

from flask_cors import CORS as BaseCORS


class CORS(object):
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this class is necessay - upstream's extension code seems to work as-is.

@@ -9,6 +9,7 @@ dill==0.2.8.2
flask==1.0.2
flask-admin==1.5.1
flask_babelex==0.9.3
flask-cors==3.0.7
Copy link
Owner

Choose a reason for hiding this comment

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

We also want to add this to setup.py's extra_requires

@@ -0,0 +1,15 @@
from .cors import CORS
Copy link
Owner

Choose a reason for hiding this comment

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

from flask_cors import CORS

should be enough :)

@briancappello briancappello force-pushed the master branch 23 times, most recently from 69507ab to 5a6f14a Compare September 28, 2019 06:47
@briancappello briancappello force-pushed the master branch 2 times, most recently from ef3c90a to c1823ca Compare September 28, 2019 08:50
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