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

Update websockets #19

Merged
merged 5 commits into from
Oct 28, 2021
Merged

Update websockets #19

merged 5 commits into from
Oct 28, 2021

Conversation

anand2312
Copy link
Contributor

Resolves supabase/supabase-py#71 by updating to websockets v9.1

Other minor changes:

Fixes some incorrect type annotations.
I have also added some TODO comments to:

@aantn
Copy link

aantn commented Oct 26, 2021

Cool, any ETA on fixing this?

@dreinon
Copy link

dreinon commented Oct 27, 2021

I would also use __future__.annotations here instead of string typing

@anand2312
Copy link
Contributor Author

👍🏻 will do

@dreinon
Copy link

dreinon commented Oct 27, 2021

@anand2312 perfect! Whenever you get the todos done, I'll review de PR 🙂

@anand2312
Copy link
Contributor Author

🤔 I don't think the TODOS will fit into the scope of this PR itself, they'd rather be separate PRs.

  • replacing get_event_loop as it is deprecated
    get_running_loop right now is not exactly the same as get_event_loop, so a proper solution needs to be thought out. Moreover, there are other things deprecated too, like a lot of stuff from typing (typing.List vs plain list etc) so that'd be it's own full size PR

  • Better error propagation
    That just needs to be well thought out :p

I added them both so that they aren't missed when new changes are being made, but I'm not free to make those changes myself right now. Perhaps I can leave issues opened.

@anand2312
Copy link
Contributor Author

It was more that I was only aiming to fix the issue at hand (upgrade websockets to a version that patches the security issue) and ended up seeing more issues in the codebase 🙂

@dreinon
Copy link

dreinon commented Oct 27, 2021

Ohh I see! I didn't understand you well. Alright 😄

@dreinon
Copy link

dreinon commented Oct 27, 2021

ping @J0 and @aantn in case you didn't understand @anand2312 well either.

@aantn
Copy link

aantn commented Oct 27, 2021

Thank you guys. I appreciate the effort.

Is there any ETA for a release on pypi including this change?

Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

looks good to me

@J0
Copy link
Contributor

J0 commented Oct 28, 2021

Thank you guys. I appreciate the effort.

Is there any ETA for a release on pypi including this change?

@aantn we will publish a new version within a few hours(after work). Thanks for your patience and understanding!

@J0 J0 merged commit 7f44217 into supabase:master Oct 28, 2021
@J0
Copy link
Contributor

J0 commented Oct 28, 2021

New release published :) Feel free to let us know if you run into any issues

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.

Security Issue: Supabase-Py pulls in a vulnerable version of websockets library
4 participants