-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Bundle download progress on Android #17809
Bundle download progress on Android #17809
Conversation
Sadly this wont work currently since progress is disabled on android (33057aa#diff-948206404ada96979d687d628e935ee1), but we've reverted this commit in the Expo fork so it would be great to merge it anyway :) |
@@ -113,7 +148,10 @@ public boolean readAllParts(ChunkCallback callback) throws IOException { | |||
Buffer chunk = new Buffer(); | |||
content.skip(chunkStart); | |||
content.read(chunk, length); | |||
emitProgress(currentHeaders, chunk.size() - currentHeadersLength, true, progressCallback); |
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.
true -> isCloseDelimiter?
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.
We do emit progress for each chunk so even if it is not the last chunk (what isCloseDelimiter means) we still want to pass isFinal to true. Its only use is to make sure the last event isn't throttled so we see 100%.
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.
If I understand this correctly, we'd reach 100% only on the last chunk -> don't we want to force the progress event only in that scenario then? Otherwise I'd say isFinal
should be named something else if it gets set to true many times and not just for the final progress event.
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.
No it is not the case, progress is reset for each chunk so let's say we have three chunks progress would look like: 0, 1, 0, 1, 0, 1. In the case of RN packager we only care about the progress of the last chunk (the JS bundle so we ignore progress of the other ones (https://github.com/facebook/react-native/pull/17809/files/bbe67864421b1de228d7af3ddd7327ed8bca060c#diff-948206404ada96979d687d628e935ee1R204).
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.
Just to clarify a chunk is a "part" from the multipart response. You can pretty much see it as multiple http responses nested inside the normal http response. It has it's own headers and we track progress of downloading each chunk separately. The goal is to allow the server to push multiple responses to the client (packager progress events + final chunk is the js bundle).
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 think I get it. I thought that the progress was for the entire multipart response (e.g. like a multipart upload) rather than for just the chunk.
What do you think about refactoring the ChunkCallback interface so that it's clear the progress events are just for that one chunk? (Breaking change to the API)
public interface ChunkListener {
/**
* Invoked when a chunk of a multipart response is fully downloaded.
*/
void onChunkComplete(Map<String, String> headers,
Buffer body,
boolean isLastChunk) throws IOException;
/**
* Invoked as bytes of the current chunk are read and may be invoked multiple times per chunk.
* This method is guaranteed to be invoked one last time when the chunk is fully downloaded,
* with `isChunkComplete` set.
*/
void onChunkProgress(Map<String, String> headers,
long chunkBytesRead,
long chunkTotalBytes,
boolean isChunkComplete) throws IOException;
}
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.
Makes sense, I agree it will be clearer like that, not sure if we have to mark this as breaking since I don't think it is meant to be public api.
emitChunk(chunk, isCloseDelimiter, callback); | ||
currentHeaders = null; |
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.
clear these only when we return (or don't clear at all) so we don't always need to reparse?
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.
Each chunk of the multipart response has it's own headers so this is why we clear it every time we emit a chunk so we parse the next one.
currentHeaders = parseHeaders(headers); | ||
} | ||
} else { | ||
emitProgress(currentHeaders, content.size() - currentHeadersLength, false, progressCallback); |
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.
Instead of currentHeadersLength (assuming we want to calculate the size of the response body downloaded so far), don't we want to track the starting index of the response body?
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.
Yea there might be an error here, we don't take into consideration the length of the headers delimiter, it can cause the progress number to be off by a bit. I could keep currentHeadersLength and do content.size() - currentHeadersLength - headersDelimiter.size()
or change currentHeadersLength for something like contentStart.
3f68664
to
60e8060
Compare
@facebook-github-bot shipit |
Something went wrong executing that command, @hramos could you take a look? |
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.
@ide is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Android equivalent of #15066 Tested that download progress shows up properly when reloading the app. [ANDROID] [FEATURE] [DevSupport] - Show bundle download progress on Android Closes #17809 Differential Revision: D6982823 Pulled By: hramos fbshipit-source-id: da01e42b8ebb1c603f4407f6bafd68e0b6b3ecba
Summary: Android equivalent of facebook#15066 Tested that download progress shows up properly when reloading the app. [ANDROID] [FEATURE] [DevSupport] - Show bundle download progress on Android Closes facebook#17809 Differential Revision: D6982823 Pulled By: hramos fbshipit-source-id: da01e42b8ebb1c603f4407f6bafd68e0b6b3ecba
Motivation
Android equivalent of #15066
Test Plan
Tested that download progress shows up properly when reloading the app.
Release Notes
[ANDROID] [FEATURE] [DevSupport] - Show bundle download progress on Android