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 library and release new major version #64

Closed
qgustavor opened this issue May 24, 2020 · 7 comments
Closed

Rewrite library and release new major version #64

qgustavor opened this issue May 24, 2020 · 7 comments

Comments

@qgustavor
Copy link
Owner

Issues like #29 and #56 will require a big rewrite so I think it's best to fix those and other long standing issues in a new major update.

Planned changes:

Additionally we can try avoiding some Node build-ins so the browser version can work as a Deno version. Currently it don't works because of this line (ironically we're still using __proto__, which is non-standard) and this line (.getReader() is not implemented, but also not needed as we use chunking). Although Deno is too young it's a good exercise to avoid cross-platform issues in future.

Questions:

  • How to handle file downloading and uploading?
    • Migrate to Streams API? Although it's interesting ― if megaFile.download() returned a ReadableStream one could run stream = megaFile.download(); textContent = await new Response(stream).text() ― Node users (the majority of library users) would need to wrap everything to work with Node Streams, which is worse than needing to wrap everything in promises, so a better option would be...
    • Keeping things as they are today and shipping a Node Stream implementation in browser code.
    • Anyway, none of those options solve Cache download URL #30 or Improve error handling #47.
    • .download() should return a readable stream, a promise that resolves to a stream, a promise that resolves to a buffer or an intermediate object like Fetch's Response?
  • What can be improved furthermore? Is better to avoid one or more of the above changes? Like, is there any downside on moving to promises? I hope not.
@jimmywarting
Copy link

jimmywarting commented Aug 14, 2020

you could also consider a bit more lower level streaming method with lower overhead instead of involving node/whatwg streams and instead use async iterable that yields Uint8Array, then it don't mather so much if you use xhr, fetch, node-fetch, https, request or deno fetch as they would yield the same result

it's also already helpful that node streams are async iterable also.

node has stream.readable.from(iterable) whatwg stream should have them eventually also but can easily be implemented with some wrapper.

// then a developer can do whatever he likes with it
for await (let uint8_chunk of downloadFile) {
  // write to disk
  // write to indexedb
  // enqueue to whatwg stream
  // push to node stream
  // write to StreamSaver.js
}
// or something like
stream.readable.from(downloadFile).pipe(fs.createWriteStream('dest.txt'))

using async iterable is more cross browser/deno/node/web worker compatible

@wiedymi
Copy link

wiedymi commented Oct 30, 2020

I could help with promisification.

@qgustavor
Copy link
Owner Author

The issue with promisification is that some parts of the library currently work with callbacks, other use streams, other use events, other use multiple. The library interface needs to be reworked.

This started as a tonistiigi/mega so it tried to keep backyards compatible as possible (IIRC only support to temporary user creation was dropped). In the tonistiigi's library the encryption and decryption stream constructors were public, but no one could use those as the library doesn't included low level functions for uploading and downloaded. That being said if some low level function will be exported I think it's better to export all of those. Like TweetNaCl.js does, low level functions don't need to be documented, just exported.

Let's take an common simplified example to think how to rewrite this: downloading a file. You can rewrite it like this: you have a high level function getMegaFile that takes a shared file URL and returns a promise that resolves to the file content and there are multiple low level functions. Just to keep it simple MAC is not verified and the file is fully download then decrypted instead of being streamed. From this example I ask:

  • What does the download function needs to return? A promise with the data? A promise that returns a stream? Something that can be used as a stream?
  • If both, how to handle both cases? An argument? Two different functions?
  • How to handle platform differences? Assume Node.js (like how it's currently developed) and use polyfills for other platforms? Write a core library without any platform dependent code and add wrappers around it? Assuming an browser-like environment (Web Crypto and fetch available, like Deno, Cloudflare Workers and, of course, browsers) is bad because Web Crypto doesn't support streaming (yet) neither AES-ECB.

MAC verification, as shown in #29, needs to be implemented separated from encryption to make download resuming easier to implement. It doesn't rely on a HMAC, instead uses a construction of AES-ECB and XOR which I don't know the name (it can be created by MEGA). WebCrypto doesn't support AES-ECB, that's why it's not implemented in the example.

@qgustavor
Copy link
Owner Author

To be fair the major reason that is making this library not being updated anymore is that I'm not interested on it. Nowadays I'm rarely using it: Direct MEGA is dead and I'm moving from MEGA in other projects because their bandwidth limits are becoming an hassle, mostly for shared IP users, which are becoming more and more common (myself included).

Currently I'm accepting pull requests as long they're constructive, follow code style and other project conventions. Some ideas from forks are good: I liked the one which adds additional promisified functions and I'm aware this library lacks a better documentation and examples.

If someone wants to rewrite the entire documentation elsewhere (currently it's in the Wiki) and add examples then, please, don't ignore review comments, follow code style, run tests and avoid typos (at least those that can be avoided with a spellchecker, every browser have one nowadays and even some Git clients!).

As this library is meant to also work in browsers I prefer that, instead of placing examples in the repository, those could be placed somewhere where those examples could be run in the browser without requiring anyone to compile code to test the library, which is something I don't like from other libraries. I CodePen is great for that as examples could be grouped in collections, although that's not great for examples that require users to be logged in. Maybe those can be handled using something like mega-mock to simulate MEGA servers, replacing the http server with a request interceptor and adding some file system emulation.

If someone with more experience with open-source projects have an better idea about how to handle this project please help me.

@qgustavor
Copy link
Owner Author

I just checked the new Mega's TOS and, beside some changes such as using "services" instead of "website" and making it clear that they're not liable if someone shares confidential content using public links, seems they're also planning to add SMS verification, which is another hassle to handle.

If they do that I will not implement it. The above comment stays true: if someone sends a pull request and it's consistent to the project organization (it follows the code style, do not break tests nor existing code, etc.), I will merge it.

@qgustavor
Copy link
Owner Author

qgustavor commented Jan 16, 2022

Since no one is commenting on this issue and I need help to decide how the library can be rewritten then I will target a middle ground:

I will replace request with fetch and release a new major version (1.0). It will not fix most issues in the library but will stop that annoying "deprecated" notice when installing the library.

Edit: and I will experiment adding .d.ts generation from JSDoc comments too.

Edit 2: there are many vulnerabilities in development packages (not in production packages) so I'll be updating or replacing some packages. As it's becoming too complex I will not add .d.ts generation.

@qgustavor
Copy link
Owner Author

The library was not rewritten, but 1.0 was released. Plans for 2.0 in this comment.

@qgustavor qgustavor unpinned this issue Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants