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

Support modern browser runtime? #11

Open
blakegong opened this issue Mar 10, 2017 · 6 comments
Open

Support modern browser runtime? #11

blakegong opened this issue Mar 10, 2017 · 6 comments

Comments

@blakegong
Copy link

blakegong commented Mar 10, 2017

Hi Rory and Linus,

Thanks for this great repo!

I am wondering whether you would like to add modern browser support? Maybe utilising FileReader API to handle fs work?

It would be great to see that happen. I can contribute with some PR too :)

Best,
Blake

@roryrjb
Copy link
Contributor

roryrjb commented Mar 10, 2017

@blakegong on one hand I wouldn't want this module to get too complicated, but on the other, having a one stop shop for MD5 check-summing in JavaScript might be really handy. Go for it.

@blakegong
Copy link
Author

Thanks for the blessing. Will try my best :)

@blakegong
Copy link
Author

Hi Rory,

Just a small update here. I didn't find enough time to do it last weekend. But managed to write a prototype using https://www.npmjs.com/package/md5 that works on the browser. Based on that experiment, there are two things that I wanna discuss here.

  1. Since crypto package only exists in node, in order to make it work in browser, would you prefer to write md5 calculation logic by hand, or simply use the aforementioned library?
  2. Personally I intend to keep the current API. Problem is, browser version of this library will introduce two additional files browser.js and promise-browser.js in order to make importing from md5-file and md5-file/promise work (exporting different code based on different node.env would make file size bigger. e.g.: node does not need all the new logics I am gonna put in this library). Thus I am hoping to get your approval to change the file structure, placing current index.js and promise.js into /node/ or /src/node/, and the same for browser code. In this way, better code splitting and clear file structure should be achieved.

@roryrjb
Copy link
Contributor

roryrjb commented Mar 15, 2017

@blakegong I wouldn't worry too much about the increased file size, the md5 package and its dependencies are tiny compared to if you used Browserify on the md5-file module itself which results in a file around 500KB. Actually talking about Browserify, I think we have to consider what happens when someone will use that method of incorporating md5-file into the browser vs. using a <script> tag for example.

@blakegong
Copy link
Author

Agreed. I myself mainly write React everyday so I guess I was underestimating the work required to make this package truly work in browser.

Guess we should support at least 3 different types of import?

  • <script> tag
  • require() as you have already done here
  • ES6 import syntax

@roryrjb
Copy link
Contributor

roryrjb commented Mar 15, 2017

@blakegong yeah sounds good

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

No branches or pull requests

2 participants