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

refactor(client/android): move fetching logic from TypeScript to Go #2221

Merged
merged 9 commits into from
Nov 4, 2024

Conversation

jyyi1
Copy link
Contributor

@jyyi1 jyyi1 commented Sep 27, 2024

New Fetch Error

image

Prerequisite: #2220

@jyyi1 jyyi1 added the do not merge Do not merge the pull request for now. label Sep 27, 2024
@github-actions github-actions bot added size/XXL and removed size/XS labels Oct 22, 2024
@jyyi1 jyyi1 force-pushed the junyi/fetch-in-go-android branch 2 times, most recently from f9bf4d8 to b033c7b Compare October 22, 2024 21:55
@github-actions github-actions bot added size/XS and removed size/XXL labels Oct 22, 2024
@jyyi1 jyyi1 marked this pull request as ready for review October 22, 2024 22:48
@jyyi1 jyyi1 requested review from a team as code owners October 22, 2024 22:48
@jyyi1 jyyi1 requested review from fortuna and sbruens October 22, 2024 22:49
} else if (Action.FETCH_RESOURCE.is(action)) {
final String url = args.getString(0);
LOG.fine(String.format(Locale.ROOT, "Fetching resource at %s ...", url));
final FetchResourceResult result = Outline.fetchResource(url);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will block. Is that a problem? I'm afraid this will freeze the UI, especially bad on timeouts.

We may need a callback mechanism instead.

Copy link
Contributor Author

@jyyi1 jyyi1 Nov 1, 2024

Choose a reason for hiding this comment

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

Aha, we both missed Line 192, 😄. All these calls are already executed in the default threadpool.

Copy link
Collaborator

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Why is the PR not blocked on approval???

@daniellacosse
Copy link
Contributor

daniellacosse commented Oct 24, 2024

Why is the PR not blocked on approval???

Cuz it's not being merged into main

Base automatically changed from junyi/fetch-in-go-electron to master November 1, 2024 23:02
@jyyi1 jyyi1 removed the do not merge Do not merge the pull request for now. label Nov 1, 2024
@jyyi1 jyyi1 requested a review from fortuna November 1, 2024 23:30
@jyyi1 jyyi1 merged commit 08678fd into master Nov 4, 2024
22 checks passed
@jyyi1 jyyi1 deleted the junyi/fetch-in-go-android branch November 4, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants