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(*): Support for send 25Mb+ files #771

Merged
merged 13 commits into from
Oct 2, 2017
Merged

Conversation

binsee
Copy link

@binsee binsee commented Aug 31, 2017

  1. Upload a file larger than 25Mb.
  2. Limit the upload file to no more than 100Mb.
  3. According to webwxapp perfect upload processing.
  4. Increase the upload and handling of upload exceptions.

Issues: #766

@binsee
Copy link
Author

binsee commented Aug 31, 2017

image

@huan huan requested a review from mukaiu August 31, 2017 15:14
@huan huan assigned hczhcz and lijiarui and unassigned hczhcz and lijiarui Aug 31, 2017
@huan huan requested review from hczhcz, lijiarui and a team August 31, 2017 15:15
@huan
Copy link
Member

huan commented Aug 31, 2017

Thank you @binsee!

This PR looks good to me. Please get more approvements from other contributors and I'll be able to merge this PR after we got three approvements.

Cheers!

@huan huan self-requested a review August 31, 2017 15:17
@huan huan requested a review from ax4 September 12, 2017 03:50
@@ -395,43 +395,111 @@ export class PuppetWeb extends Puppet {
// Sending video files is not allowed to exceed 20MB
// https://github.com/Chatie/webwx-app-tracker/blob/7c59d35c6ea0cff38426a4c5c912a086c4c512b2/formatted/webwxApp.js#L1115
const videoMaxSize = 20 * 1024 * 1024
Copy link
Member

Choose a reason for hiding this comment

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

maxVideoSize, largeFileSize, maxFileSize would be better variable names

const id = 'WU_FILE_' + this.fileId
this.fileId++

const hostname = await this.browser.hostname()
const headers = {
Referer: `https://${hostname}`,
'User-Agent': 'Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36',
Copy link
Member

Choose a reason for hiding this comment

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

Just curious.. Why does the default UA fail here?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I did not test other UA.

@huan
Copy link
Member

huan commented Sep 24, 2017

Awesome, thanks all for the code review which will help the Wechaty better!

@binsee Could you please resolve the conflicts in your PR? I'll be able to merge it after that.

@binsee
Copy link
Author

binsee commented Sep 24, 2017

I will deal with this problem as soon as possible.

@binsee
Copy link
Author

binsee commented Oct 2, 2017

I made a mistake...
I'll re-send a new pr...

@binsee binsee closed this Oct 2, 2017
@huan
Copy link
Member

huan commented Oct 2, 2017

Ok. But it's always a good behavior to update one PR till be merged, because close and open a new PR will lose all the comments/reviews.

So I'd like to suggest you reopen this PR and keep updating this one. :)

@binsee
Copy link
Author

binsee commented Oct 2, 2017

@zixia new pr: #842

I am wrongly merge the master branch, and the change record is very large, which will lead to the review becomes more difficult.

Use the latest code to cover the pr branch, I do not know what will lead to the results?

@binsee binsee reopened this Oct 2, 2017
@huan
Copy link
Member

huan commented Oct 2, 2017

After you sync with the master branch, I suggest you do a git rebase to rebase your PR on the top of the latest changes.

Then I believe this PR will be ok again.

Could you have a try with git rebase and git push?

1. Upload a file larger than 25Mb.
2. Limit the upload file to no more than 100Mb.
3. According to webwxapp perfect upload processing.
4. Increase the upload and handling of upload exceptions.
binsee added 10 commits October 2, 2017 12:00
1. add MediaMessage.saveFile() , used to save attachment
2. Refactor MediaMessage.forward(), support send to Room and Contact
array, add support download big file and send it again.
Because webWx restrictions, more than 25 MB of files can not be
downloaded, it can not be forwarded.
@binsee
Copy link
Author

binsee commented Oct 2, 2017

@zixia Success, thanks for the reminder.

@@ -461,7 +539,7 @@ export class PuppetWeb extends Puppet {
mediatype,
uploadmediarequest: JSON.stringify(uploadMediaRequest),
webwx_data_ticket: webwxDataTicket,
pass_ticket: passTicket,
pass_ticket: passTicket || '',
Copy link
Author

Choose a reason for hiding this comment

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

After testing, this parameter is not required when uploading a file.

Copy link
Author

Choose a reason for hiding this comment

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

fix #777

* It should be noted that when forwarding ATTACH type message, if the file size is greater than 25Mb, the forwarding will fail.
* The reason is that the server shields the web wx to download more than 25Mb files with a file size of 0.
*
* But if the file is uploaded by you using wechaty, you can forward it.
Copy link
Author

Choose a reason for hiding this comment

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

Note that this is a new feature that supports forwarding your own uploaded 25Mb + files.

*
* ```javasrcipt
* .on('message', async m => {
* if (m.self() && m.rawObj && m.rawObj.Signature) {
Copy link
Author

Choose a reason for hiding this comment

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

Here is the code to forward the 25mb+ file.

Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome fix!

Please let me know when you have been ready for merging.

src/message.ts Outdated
ret = <boolean>await new Promise((resolve, reject) => {
readStream.pipe(writeStream)
readStream
.on('end', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Use once('end') at here should be better?

src/message.ts Outdated
readStream
.on('end', () => {
log.silly('MediaMessage', `saveFile() pipe end`)
resolve(true)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to suggest move resolve() to the following code:

writeStream.once('close', resolve)

and also add this to monitor failure event:

writeStream.once('error', reject)

src/message.ts Outdated
*
* @param filePath save file
*/
public async saveFile(filePath: string): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

About the return value boolean:

I'd like to suggest to use void as the return value. If so, we might want to throw an Error if the operation failed.

Copy link
Author

Choose a reason for hiding this comment

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

It has been modified.

@binsee
Copy link
Author

binsee commented Oct 2, 2017

@zixia
There is no other need to modify the place, you can merge.
Thanks!

@huan huan merged commit 4320003 into wechaty:master Oct 2, 2017
@huan
Copy link
Member

huan commented Oct 2, 2017

Fantastic! Thank you for the great work!

@binsee binsee deleted the send-25mb-file branch October 2, 2017 10:43
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

4 participants