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

re-implement simple CORS middleware for blackd #2500

Merged
merged 7 commits into from Sep 25, 2021
Merged

Conversation

zsol
Copy link
Collaborator

@zsol zsol commented Sep 18, 2021

Description

This PR removes Black's dependency on aiohttp-cors because it seems to be unmaintained and spewing DeprecationWarnings (due to the use of @coroutine decorators).

TODO

  • Add a CHANGELOG entry
  • Add tests
  • Actually remove aiohttp-cors from setup.py
  • Actually remove aiohttp-cors from Pipfile


return resp

return impl # type: ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mypy can't figure out the type of impl here unfortunately - it complains about returning an Any value instead of Middleware. @JelleZijlstra any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What version of aiohttp are we using? On master (https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_middlewares.py#L34) it looks like the decorator does nothing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like 3.6 and above:

black/setup.py

Line 89 in d08f95a

"d": ["aiohttp>=3.6.0"],

We could update this, but I'd prefer doing it in a separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

For what it's worth the decorator does do something on aiohttp 3.6: https://github.com/aio-libs/aiohttp/blob/3.6/aiohttp/web_middlewares.py#L34. I'm not sure why mypy turns the function into Any here though; the type annotations seem like they should work.

@ichard26
Copy link
Collaborator

I'm very concerned about the changes within Pipfile.lock. I suspect pipenv removed too much, eg. dataclasses. I have some experience (un)fortunately in this regard and I'll try to reduce the lockfile changes to avoid breakage.

@zsol
Copy link
Collaborator Author

zsol commented Sep 18, 2021

You're right, dataclasses got downgraded to 0.6 accidentally, let me see if I can fix that

@zsol
Copy link
Collaborator Author

zsol commented Sep 18, 2021

I gave up on trying to get pipenv to not revert the dataclasses version :)

zsol and others added 5 commits September 18, 2021 22:47
Although there's a little bit of extra changes. They should be fine tho.
Dataclasses is still broken on Python 3.7+ but I'll fix that after this.
@ichard26
Copy link
Collaborator

I wasn't able to get the cleanest Pipfile.lock diff ever but this should do for the time being (ignoring the dataclasses fiasco).

@ichard26 ichard26 linked an issue Sep 19, 2021 that may be closed by this pull request
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

This looks fine to me but the only knowledge of CORS I have comes from a quick read of the MDN documentation. I hope someone else with more web dev experience can pitch in.

@zsol zsol merged commit a5381ba into main Sep 25, 2021
@zsol zsol deleted the remove-aiohttp-cors branch September 25, 2021 11:58
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.

aiohttp-cors dependency
3 participants