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

Fix token expiry date #33

Closed
wants to merge 7 commits into from

Conversation

emin-grbo
Copy link
Contributor

As detailed in this issue, token was getting new date on every encode/decode, therefore never expiring.

This PR omits setting the expiresAt date and tries to only set it during first authentication and refresh.

After this, we might want to explore the option of a force refresh if 401 response is received.
That way expiry would never really need to be logged as we can always try with the old token and refresh if the bad response is received.

@emin-grbo emin-grbo mentioned this pull request Jun 16, 2022
Copy link
Owner

@daneden daneden left a comment

Choose a reason for hiding this comment

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

I think you're missing some upstream changes too; check out the changes in your PR to Twift.swift.

Also see my comment on #32; I think we need to encode expiresAt since expiresIn by itself is no use when decoding at an arbitrary later date.

Comment on lines +50 to +56
internal func fieldsOnly<T: Fielded>(for type: T.Type, fields: Set<T.Field>) -> [URLQueryItem] {
var queryItems: [URLQueryItem] = []

if !fields.isEmpty { queryItems.append(URLQueryItem(name: T.fieldParameterName, value: fields.compactMap { T.fieldName(field: $0) }.joined(separator: ","))) }

return queryItems
}
Copy link
Owner

Choose a reason for hiding this comment

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

What's the advantage of this over fieldsAndExpansions?

Comment on lines 17 to +18
fields: Set<Tweet.Field> = [],
userFields: Set<User.Field> = [],
Copy link
Owner

Choose a reason for hiding this comment

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

I think I see what's happened here; we should make fields a Set<List.Field> instead of introducing a userFields property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, never made these changes myself 😂 messed up the update and had some local git issues.

@emin-grbo
Copy link
Contributor Author

Oh wow, messed up the PR completely, I was sure I checked but it's not up to date at all. I will close it and re-do in the next few days 🙌

@emin-grbo emin-grbo closed this Jun 19, 2022
@emin-grbo
Copy link
Contributor Author

I just came back from the vacation, sry to have completely spaced this 😅, never got to correcting the PR.
Did you handle anything on this front or should I try to have at it again?

@daneden
Copy link
Owner

daneden commented Jul 2, 2022

I think I fixed this in 2d518d1, feel free to verify in your project!

@emin-grbo
Copy link
Contributor Author

ohhh this looks nice, will check 🙌

@emin-grbo emin-grbo deleted the fix-token-expiry-date branch August 10, 2022 17:42
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.

None yet

2 participants