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

Streaming broken? #44

Closed
andig opened this issue Mar 16, 2021 · 4 comments · Fixed by #45
Closed

Streaming broken? #44

andig opened this issue Mar 16, 2021 · 4 comments · Fixed by #45
Assignees
Labels
bug Something isn't working

Comments

@andig
Copy link
Collaborator

andig commented Mar 16, 2021

The streaming API seems to require basic auth rather than bearer token:

https://github.com/bogosj/tesla/blob/main/stream.go#L38

Original code:

https://github.com/jsgoecke/tesla/blob/e02ebd220e5a4158d790d7ed857f937f7f53439e/stream.go#L38

Currently, the basic auth is overridden by the oauth transport. Is that what we need here?

UDPATE seems we need to store the non-oauth client somewhere and use that here, reverting to the original code above. I'll try to prepare something.

@bogosj
Copy link
Owner

bogosj commented Mar 16, 2021

Entirely possible, I've never used the streaming API. If this is in fact broken we should fix it before #16

@andig andig added the bug Something isn't working label Mar 16, 2021
@andig
Copy link
Collaborator Author

andig commented Mar 16, 2021

It looks as if the current streaming implementation is entirely broken. It apparently needs to use web sockets: https://github.com/timdorr/tesla-api/commits/d6bb6ff7fe46d62aa6b982bb5e9adb09861bc705/lib/tesla_api/stream.rb.
This change happened in 2019.

For 1.0 we should remove the public API, but leave the stream files in for fixing later?

@bogosj
Copy link
Owner

bogosj commented Mar 16, 2021

I support un-exporting those symbols and leave it to someone who has an interest in the streaming API to drive by and fix it :)

@bogosj
Copy link
Owner

bogosj commented Mar 16, 2021

I propose we just remove all references to the streaming API and leave a note behind in the README.md that it was removed in the squash/merge commit I create from the PR above.

@bogosj bogosj self-assigned this Mar 16, 2021
@andig andig mentioned this issue Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants