-
-
Notifications
You must be signed in to change notification settings - Fork 589
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
We don't care about the timeline (just fire and forget to load the event) #2849
We don't care about the timeline (just fire and forget to load the event) #2849
Conversation
Split out from #2521 Context: #2521 (comment)
await this.client.getEventTimeline(this.timelineSet, initialEventId) | ||
.then(initFields); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function signature is Promise<void>
here. We shouldn't return anything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. It doesn't look like this changes any behavior?
return this.client.getEventTimeline(this.timelineSet, initialEventId) | ||
await this.client.getEventTimeline(this.timelineSet, initialEventId) | ||
.then(initFields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.client.getEventTimeline(...).then(initFields)
was already a Promise<void>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think I confused my sentiment with this spot (which is totally different, oops) that I mentioned in #2521 (comment) with some blind copy-pasting out of that PR diff. I'm trying to get back to speed with everything going on in that stale PR.
And if we actually wanted to refactor to a fire and forget getEventTimeline
pattern here, I'd need to adjust how initFields
is done here.
I'll have a think on this one whether it's relevant. Good shout!
removing review requests while this is in draft |
We don't care about the timeline (just fire and forget to load the event).
Split out from #2521
Checklist
Sign-off given on the changes (see CONTRIBUTING.md)This change is marked as an internal change (Task), so will not be included in the changelog.