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

Rewrite data fetch using async/await. Refs #25 #32

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

Ryuno-Ki
Copy link
Contributor

@Ryuno-Ki Ryuno-Ki commented Apr 8, 2021

Signed-off-by: André Jaenisch [email protected]

I wasn't sure, how far you want to have async/await applied.
(See also What Color is Your Function?

Copy link
Collaborator

@fluffy-critter fluffy-critter left a comment

Choose a reason for hiding this comment

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

I was thinking that the whole thing could just avoid using explicit promises and remove the getData URL entirely, and the main flow would be like:

let response = await window.fetch(url);
if (response.status < 200 || response.status >= 300) {
    throw "data fetch failed";
}
// do the rest of the stuff to response.json

like, the only reason getData existed is to support the XHR polyfill.

Ryuno-Ki added 2 commits April 8, 2021 22:04
Signed-off-by: André Jaenisch <[email protected]>
Signed-off-by: André Jaenisch <[email protected]>
@Ryuno-Ki Ryuno-Ki requested a review from fluffy-critter April 8, 2021 20:06
Copy link
Collaborator

@fluffy-critter fluffy-critter left a comment

Choose a reason for hiding this comment

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

Just a nitpick about a comment that I overlooked earlier, otherwise it's fine

};

json.children.forEach(function(child) {
// This seem to push the reaction into either comments or collects
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make this comment "put the reaction into its appropriate destination"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added that comment, because I wasn't quite sure what's going on there.
Feels a bit dirty to have the store point to the respective object in memory …
=> Another issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is nothing dirty about that pattern. It's simple and straightforward and efficient.

@fluffy-critter fluffy-critter merged commit 08ae4ab into PlaidWeb:main Apr 8, 2021
@Ryuno-Ki Ryuno-Ki deleted the async-await branch April 9, 2021 16: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.

2 participants