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

🔨 Resolve prices.tf 401 issue #1283

Merged
merged 3 commits into from
Aug 1, 2022
Merged

🔨 Resolve prices.tf 401 issue #1283

merged 3 commits into from
Aug 1, 2022

Conversation

idinium96
Copy link
Member

@idinium96 idinium96 commented Aug 1, 2022

Might fix the issue where the bot doesn't even bother to get new JWT from prices.tf. Quite ugly tbh.

image

@idinium96 idinium96 requested review from RobotoLev and joekiller and removed request for RobotoLev August 1, 2022 19:42
@RobotoLev
Copy link
Collaborator

Reviewing it rn

@idinium96
Copy link
Member Author

Hhmmmm something is wrong. It does not wait until one job is completed.

image

image

image

await this.setupToken();
this.setupToken()
.then(() => {
return PricesTfApi.apiRequest<B>(httpMethod, path, params, data, {
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably because of this... Let me try remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm kinda necessary tbh

@RobotoLev
Copy link
Collaborator

I had that situtation today, so I'm watching the log in order to make something more clear. I don't think that happens because of using try-catch instead of promise's .catch() thing.

@idinium96
Copy link
Member Author

idinium96 commented Aug 1, 2022

I had that situtation today, so I'm watching the log in order to make something more clear. I don't think that happens because of using try-catch instead of promise's .catch() thing.

It's okay I've found the problem. But now another problem came (but probably off-topic)

@idinium96
Copy link
Member Author

Looks like this doesn't help either. It doesn't even update my dummy token.
image

@RobotoLev
Copy link
Collaborator

So as I thought, it just doesn't re-request token for some reason. I'm pretty sure all was fine when I was implementing fix for v4.13.1.

As I see in my Discord channel history, this strange 401 wasn't appearing on v4.13.1-v5.0.0, but appeared again at v5.0.1. I still don't know what might be reason for it, but that's the pure fact.

@idinium96
Copy link
Member Author

So as I thought, it just doesn't re-request token for some reason. I'm pretty sure all was fine when I was implementing fix for v4.13.1.

As I see in my Discord channel history, this strange 401 wasn't appearing on v4.13.1-v5.0.0, but appeared again at v5.0.1. I still don't know what might be reason for it, but that's the pure fact.

For me, it started appearing on v4.11.0.
I think I am going to revert changes made in 3cbf725

@RobotoLev
Copy link
Collaborator

Wait a second. Can filterAxiosError change error format???

@RobotoLev
Copy link
Collaborator

You've probably cut off response from AxiosError, so catch doesn't re-request token.

@idinium96
Copy link
Member Author

Wait a second. Can filterAxiosError change error format???

Ah dang, you're right. That might be the issue. 🤐

@RobotoLev
Copy link
Collaborator

That's definitely it. There's no err.response.status now (there's no err.response even), so bot thinks that it's non-standart error and drops it farther.

@idinium96
Copy link
Member Author

That's definitely it. There's no err.response.status now (there's no err.response even), so bot thinks that it's non-standart error and drops it farther.

Let me try once again, and then again before this.

@idinium96
Copy link
Member Author

Yeah... gonna try the old one.
image

@idinium96
Copy link
Member Author

Oh god, the old one works just fine
image

@RobotoLev
Copy link
Collaborator

That's me who was implementing it (check v4.13.1). At that time 401 was handled well, so now I knew the problem is not here.

@joekiller
Copy link
Collaborator

Yeah I'd revert. Because the function expects a promise returned but nothing is happening but a void call who knows when it'll resolve. It is likely executing out of order. If you want to keep the new form (but using callbacks blah that's what async is for) you'd need to return the promise instead of executing it as a void

@idinium96
Copy link
Member Author

That's me who was implementing it (check v4.13.1). At that time 401 was handled well, so now I knew the problem is not here.

Yeah, that was before the filterAxios thing.

@idinium96
Copy link
Member Author

Yeah I'd revert. Because the function expects a promise returned but nothing is happening but a void call who knows when it'll resolve. It is likely executing out of order. If you want to keep the new form (but using callbacks blah that's what async is for) you'd need to return the promise instead of executing it as a void

Yeah gonna revert it now. Thank you :3

@idinium96
Copy link
Member Author

Thank you @RobotoLev!
Feel free to approve this PR.
image

@idinium96 idinium96 merged commit 3e5248c into development Aug 1, 2022
@idinium96 idinium96 mentioned this pull request Aug 1, 2022
@idinium96 idinium96 deleted the 401-issue branch August 1, 2022 23:16
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.

3 participants