Uh oh!
There was an error while loading. Please reload this page.
Conversation
stephenmathieson
left a comment
There was a problem hiding this comment.
LGTM! This is great dude!
vadimdemedes
left a comment
There was a problem hiding this comment.
Should just be uuid. uuidv4 is ugly.
f2prateek
commented
Jan 17, 2018
@vadimdemedes I just copy pasted that from their docs: https://github.com/kelektiv/node-uuid |
@f2prateek them writing it that way in the readme doesn't mean it's the best way to do it :) |
f2prateek
commented
Jan 17, 2018
Will change |
Both use the node crypto package on Node apps so there shouldn't be a meaningful difference here. The differences come in the browser. crypto-token works in the browser by polyfilling the crypto package. node-uuid works by using the browser crypto apis if available, otherwise falling back to Math.random() (https://github.com/kelektiv/node-uuid/blob/master/lib/rng-browser.js). Because of polyfilling on the browser, crypto-token contributes 90kb to our library in the browser which is rougly 80% of the library size on the browser (the rest of the library is only 20kb). node-uuid only adds 1kb (though I believe this comes at the cost of generating slightly less random messageIds on the browser). I don't think the extra weight is worth the cost, especially as we're not using this in a critical security context. One thing we now do to increase randomness in the browser is includes a hash of the contents of the message as well like we do in a.js (https://github.com/segment-integrations/analytics.js-integration-segmentio/blob/master/lib/index.js#L256). On a side note, analytics.js also uses the uuid package. Before: Package size: 107.79 KB Size limit: 150 KB With all dependencies, minified and gzipped After: Package size: 13.79 KB Size limit: 150 KB With all dependencies, minified and gzipped Because of the significant reduction in size, I also updated the size limit to be 25kb instead of 150kb.
f2prateek
commented
Jan 17, 2018
@vadimdemedes Updated the Side Note: E2E tests are being flaky because of some destinations downtime today, so if Circle fails I'll kick it off again. |
Codecov Report
@@ Coverage Diff @@## master #150 +/- ##
=====================================
Coverage 100% 100% =====================================
Files 1 1 Lines 96 97 +1 =====================================
+ Hits 96 97 +1
Continue to review full report at Codecov.
|
Both use the node crypto package on Node apps so there shouldn't be a meaningful difference here.
The differences come in the browser. crypto-token works in the browser by polyfilling the crypto package. node-uuid works by using the browser crypto apis if available, otherwise falling back to Math.random() (https://github.com/kelektiv/node-uuid/blob/master/lib/rng-browser.js).
Because of polyfilling on the browser, crypto-token contributes 90kb to our library in the browser which is rougly 80% of the library size on the browser (the rest of the library is only 20kb). node-uuid only adds 1kb (though I believe this comes at the cost of generating slightly less random messageIds on the browser). I don't think the extra weight is worth the cost, especially as we're not using this in a critical security context.
One thing we now do to increase randomness in the browser is includes a hash of the contents of the message as well like we do in a.js (https://github.com/segment-integrations/analytics.js-integration-segmentio/blob/master/lib/index.js#L256). On a side note, analytics.js also uses the uuid package.
Before:
Package size: 107.79 KB
With all dependencies, minified and gzipped
After:
Package size: 13.79 KB
With all dependencies, minified and gzipped
Because of the significant reduction in size, I also updated the size limit to be 25kb instead of 150kb.