-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Replace request with fetch (and fix other bugs) #86
Conversation
Now async/await support is required for building. As esbuild do not compile to ES5 old browsers are not supported at this moment. Tests are failing, probably the code that handles uploading got broken in 486cd9d.
I updated the branch with some fixes, I tested in Node and I could download some files. Both browser and Deno are not working well, but I will put those aside for the moment. There are still a important pending issue: downloading only works reliably when using single-connection downloading, trying to use chunked downing results in the server disconnecting ( I'm checking megatools source-code and the logic is pretty the same, but it retries failed chunks. How about creating something to handle chunk errors like megatools? If so, how? Hardcode a exponential backoff like megatools or make something more configurable? Other possibility is trying to detect what is causing the server to drop the connection. In those changes I added a HTTP(S) agent to keep the connection alive and a default user agent (which should be used anyway), but the issue persists. Edit: Travis finished, for some reason tests are failing in Node 14 with |
Update Travis Node versions. Make the API easier to be overridden. Use a default user-agent and HTTP(S) agent. Implement again single-connection downloading as is the only reliable way to download at the moment. Fix the browser version to, at least, load file attributes.
I will add a function handleRetries (tries, error, cb) {
if (tries > 8) {
cb(error)
} else {
setTimeout(cb, 1000 * Math.pow(2, tries))
}
} The first argument is the current number of tries, the second is the last error and the third is a callback. Calling the callback with an argument will throw an error in the stream and stop the download. The default logic is similar to the one in megatools but instead of capping the max wait to 4 minutes, it stops the download. By making it an argument library users can override this logic to match their needs without having to modify the library code. A similar argument can be added to the upload function too. Edit 1: If someone thinks a function like that needs more arguments or, maybe, even replacing Edit 2: The default retry handler can be overridden globally by replacing |
Tests are getting a lot of warnings because of this: node-fetch/node-fetch#1446 I will wait this issue to be fixed first. Edit 1: I downgraded node-fetch to v2 in order to resume development of this library, I will return to v3 when the above issue gets fixed. Ava tests are passing without warnings now. In the other hand Ava's tests use the mock server, not real servers, and for some reason they continue hanging up. Retrying chunks sure helped to reduce the number of failed downloads, but there are some cases when some chunks fail more than eight times, so while it was an improvement, it was not the solution. I will need to do more tests first to guess what's going on. Edit 2: Node's own debugger do not support debugging HTTP(S) requests. Using a debugging proxy changes the behavior (it worked fine when I attached Fiddler). I will try to fix the browser build and debug using a browser. |
And remove Node 14 from Travis. I don't know why it's failing and I don't want to fix it.
The code now looks good for me. The only thing missing is node-fetch issue to be fixed. Would be great if someone test this branch. Edit: I think that's better to try fixing more issues before publishing, like fixing those CI checks that are failing. The issue is that Ava's testing the source code, not compiled esbuild code, using Babel and Babel is failing on Node 14. Is better to rewrite the tests to use only compiled esbuild code, Some internal functions are not exported, so I will need to make esbuild export a build just for testing that include them. Is better to remove the current browser test code as it's just testing the pure-JavaScript versions of crypto functions and add a test using real browsers later. Edit 2: I think will be better to replace Travis with Github Actions. From my experience it works better. Also, while a test with real browsers may come later, testing with Deno seems to be a good thing as it's similar to browsers: the only things that change between Node and browser builds is crypto code and HTTP requesting code, and in those two cases the same code used for browsers also work in Deno. Edit 3: Is not like someone really depends on this project or cares for it - except me - so I will take my own time to fix it. |
The idea behind those changes is removing Babel dependencies by making Ava work using ES modules, but them I had to rename everything from .js to .mjs. I could add type: 'module' to package.json but them I think it could cause some issues to users of the library that were using it as a commonjs module. Well, now the source code is a es module that can be run without any compiling. I wonder if would be better to just export the source code as the module build and just compile for cjs and browsers/deno. Better not: currently using esbuild allows to switch to typescript quite easily. Browser tests removed: they were doing nothing important, just testing pure-javascript fallbacks, but currently the browser build is failing because the browser version of streams is not working well. Will be replaced with Deno based tests later as Deno is quite close to browsers but do not have the issue of being as slow and resource intensive as a browser. Moved from Travis to Github Actions.
Just a reminder to myself that implementing MEGA's weird MAC using native functions is not only hard, but makes the code run slower (at least on Node 16). It's similar to this: https://en.wikipedia.org/wiki/CBC-MAC But there is a small change on this that makes things harder: the computation result is added to a second ECBC-MAC and the main one resets every chunk. Also, for some reason, trying to remove this.macs = [] by calculating the final mac in the checkMacBounding function made it slower too. The bad thing is that it increases memory usage, a 16 byte Buffer every 1 MB or so. If someone wants to fix those things, do the benchmarks and send a pull request.
Fixes #29. Finally. After FOUR years.
Package-lock.json was removed because Travis on 04cd018. It's returning because GitHub Actions. I hope it don't start causing issues again. Otherwise I will return to Travis!
I expected to Github Actions to be better. And it is: now I know why Node 14 was failing. Or not, it should not, before Babel was compiling the code so the require "hack" was not an issue.
But is needed for the greater well being of everyone
Now, from the issues I want to fix, only Typescript related ones are missing. Nice. And I need to sleep. Edit: Nevermind, I still need to fix the browser build and make Deno-based tests work. Edit 2: Also need to add tests for the logout and verification methods. Also move documentation from the wiki to something else where versioning is supported. |
Also reduce timeout duration, it was too long.
Tests will be built using esbuild and this file will be replaced with a Deno compatible version when testing. By compiling using esbuild migrating to Typescript should be easier.
Storage and stream encryption tests can be run on built code, other tests will need to be compiled from source.
Also export stream.mac (otherwise implementing this test would be quite a headache).
And remove `return this`
For some reason ts-typescript is not working with the ESM types: "Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser." Because of that this file is not being linted at the moment.
I could not resist and I fixed the issue with Deno types. The documentation is updated. I did some simple tests and everything is working well, it's even type checking events, which is great. |
- Handle AbortError in the API - Make stream errors synchronous - Support single-stream download in browser/Deno - Handle download cancelling in single-stream downloading - Remove unneeded process.nextTick since promises are being used - Simplify promisify function - Add tests
If you call sml while connections are still open then they will return ESID. But aborting is not enough, so API ignores responses after it was been closed.
Seems chunked download is broken: I just got two corrupted downloads. I think the issue is how the retry mechanism works since storage testing do not test it (mega-mock do not simulate a failed connection). Edit: well, there is no test downloading the large file uploaded before, so the issue might be other thing. I will change this test to include some deterministic random looking data instead of zeros to detect if chunks are merged in the wrong order. Edit 2: I'm modifying the streamed upload test to also test downloading... and it's failing. Edit 3: streamed uploading and chucked downloading are broken. Edit 4: for some reason Edit 5: Checking documentation I checked the Edit 6: Seems it will be better to rewrite the upload function to not depend on neither Edit 7: I changed the upload function to not depend on Edit 8: When I try to debug it using Node's debugger the bug vanishes. This is a Heisenbug! |
Another issue: error handling in Edit: That will be the fix. |
Also fix bandwidth limit error and error handling when using streams.
There is still an issue that's causing issues in Deno and browsers, for some reason chunks are being read out of order randomly.
Looks like there is a race-condition in the download function which causes chunks to be read out of order. Weird. |
Wrong variable. Also handle backpressure.
Looks like streaming upload and/or download are randomly failing. It needs more tests. |
Fix potential race condition. Split upload and download tests.
As issue #64 is stale I plan to at least replace request with fetch to remove the warning that NPM shows when installing the library.
Because the existent build process is old it was causing some issues, so I replaced Rollup with esbuild and updated some dev dependencies to reduce the number of reported vulnerabilities on them to zero.
But, because of that, now the distributed code is not ES5 but ES6 and it require more modern platforms, both Node and browser. Anyway, the important thing is making the library work, adding ES5 support, if someone wishes it, will come later.
Checklist:
PassThrough
stream, unlike npm's through, do not stop data from passing thought via a pipe;end
orcomplete
functions17.0.0
standard/eslint-config-standard#208 then fix issues found in files that could not be linted;