Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Use sha.js instead of full crypto in browser #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pelle
Copy link

@pelle pelle commented Aug 21, 2016

Only use sha.js per suggestions from @dignifiedquire here ipfs-inactive/js-ipfs-http-client#353

Not sure how much of an effect it will have on the distribution size

@pelle
Copy link
Author

pelle commented Aug 21, 2016

Failure in Travis is due to lint complaining about digest function names:

'''
49:10 error Identifier 'gsha2_256' is not in camel case camelcase
53:10 error Identifier 'gsha2_512' is not in camel case camelcase
'''

I suppose it could be fixed, since I guess these aren't called externally.

@dignifiedquire
Copy link
Member

To make this really efficient we need to do a bit more work, isNode is an npm module, and not a static variable, that means the require('crypto') does not get optimised away.

What we want is something like this

let createHash

if (buildingForBrowser) {
  createHash = require('sha.js')
} else {
  createHash = require('crypto').createHash
}

and then let our build tool define buildingForBrowser. This way when building for the browser this would be transformed into

let createHash

if (true) {
  createHash = require('sha.js')
} else {
  // unreachable code
  createHash = require('crypto').createHash
}

This in turn would be then properly analyzable by webpack & uglify and they could remove the unreachable code.

dignifiedquire added a commit that referenced this pull request Sep 22, 2016
In Node.js the underlying hashing functions have not changed, but the browser now uses `webcrypto` instead of JavaScript based methods for `SHA1`, `SHA2-256` and `SHA2-512`.

Also `SHA3` support was added in both Node.js and the browser.

BREAKING CHANGE:

The api was changed to be callback based, as webcrypto only exposes async methods.

Closes #10
@dignifiedquire
Copy link
Member

@pelle I just finished #12 which uses webcrypto instead of JavaScript fallbacks in the browser reducing the size even further :)

@daviddias
Copy link
Member

@pelle did you had the chance to check @dignifiedquire suggestions?

@pelle
Copy link
Author

pelle commented Nov 21, 2016

Sorry I missed this. I guess I was at devcon and all was a blur. Having a look now.

@pelle
Copy link
Author

pelle commented Nov 21, 2016

Looks great. Great progress @dignifiedquire and @diasdavid

@kumavis
Copy link

kumavis commented Nov 25, 2016

can also set crypto: false in the browser field in package.json

@daviddias daviddias added the status/ready Ready to be worked label Dec 5, 2016
@daviddias daviddias added status/deferred Conscious decision to pause or backlog and removed js-ipfs-ready status/ready Ready to be worked labels Jan 29, 2017
@daviddias daviddias added status/ready Ready to be worked exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P3 Low: Not priority right now and removed status/deferred Conscious decision to pause or backlog labels Oct 17, 2017
@hacdias
Copy link
Member

hacdias commented Jan 2, 2018

@diasdavid I think this one could be closed since it is a bit old, has a lot of conflicts, and js-multihashing is using webcrypto now.

@daviddias
Copy link
Member

@hacdias only multihashing-async is using webcrypto, not this one.

@hacdias
Copy link
Member

hacdias commented Jan 2, 2018 via email

@daviddias
Copy link
Member

@hacdias the thing is that you can't. WebCrypto is async only (that is why there is a multihashing-async)

@daviddias daviddias added exp/expert Having worked on the specific codebase is important and removed exp/novice Someone with a little familiarity can pick up labels Jan 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue P3 Low: Not priority right now status/ready Ready to be worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants