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.

Sign up for GitHub

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

Use isomorphic-webcrypto (enable React Native). #43

Closed
wants to merge 3 commits into from
Closed

Use isomorphic-webcrypto (enable React Native). #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e7bbba
Use isomorphic-webcrypto (for use with React Native).
dmitrizagidulin May 8, 2021
00f496a
Make lint pass.
dmitrizagidulin May 8, 2021
13286c6
Add a react-native section to package.json, split off from browser file.
dmitrizagidulin May 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/benchmark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ function addTest(manifest, test) {
}

// expand @id and input base
const test_id = test['@id'] || test['id'];
const test_id = test['@id'] || test.id;
test['@id'] = manifest.baseIri + basename(manifest.filename) + test_id;
test.base = manifest.baseIri + test.input;
test.manifest = manifest;
Expand Down
6 changes: 1 addition & 5 deletions lib/MessageDigest-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,8 @@
*/
'use strict';

require('setimmediate');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that this was put here, as I think it may have been required for some other reason (i.e., this function isn't supported in browsers but we use it elsewhere). @davidlehn, can you speak to this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setImmediate is used in the code, so a polyfill is needed. See lib/URDNA2015.js. This polyfill replaced custom code and was added in:
4e10528

The browser testing of this package is indirect through jsonld.js, to avoid complexity of duplicating the testing framework. A side effect of that seems to be confusion as to why this is needed. jsonld.js tests will fail without this.

I'm not sure why it's in this file in particular. I probably thought it easier to just use this file rather than create a new empty node file and a browser only file with just that require.

Please restore. If a comment or moving to a new browser only file would help avoid confusion, that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh got it, thanks!!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidlehn interestingly, requiring setimmediate causes to fail a build in my case. I could provide a repro if you are interested and would be interested in understanding why.


const crypto = self.crypto || self.msCrypto;

// TODO: synchronous version no longer supported in browser

module.exports = class MessageDigest {
/**
* Creates a new MessageDigest.
Expand All @@ -26,7 +22,7 @@ module.exports = class MessageDigest {
} else if(algorithm === 'sha1') {
this.algorithm = {name: 'SHA-1'};
} else {
throw new Error(`Unsupport algorithm "${algorithm}".`);
throw new Error(`Unsupported algorithm "${algorithm}".`);
}
this._content = '';
}
Expand Down
46 changes: 46 additions & 0 deletions lib/MessageDigest-reactnative.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Copyright (c) 2021 Digital Bazaar, Inc. All rights reserved.
*/
'use strict';

const crypto = require('isomorphic-webcrypto');
require('fast-text-encoding');

module.exports = class MessageDigest {
/**
* Creates a new MessageDigest.
*
* @param algorithm the algorithm to use.
*/
constructor(algorithm) {
// check if crypto.subtle is available
// check is here rather than top-level to only fail if class is used
if(!(crypto && crypto.subtle)) {
throw new Error('crypto.subtle not found.');
}
if(algorithm === 'sha256') {
this.algorithm = {name: 'SHA-256'};
} else if(algorithm === 'sha1') {
this.algorithm = {name: 'SHA-1'};
} else {
throw new Error(`Unsupported algorithm "${algorithm}".`);
}
this._content = '';
}

update(msg) {
this._content += msg;
}

async digest() {
const data = new TextEncoder().encode(this._content);
const buffer = new Uint8Array(
await crypto.subtle.digest(this.algorithm, data));
// return digest in hex
let hex = '';
for(let i = 0; i < buffer.length; ++i) {
hex += buffer[i].toString(16).padStart(2, '0');
}
return hex;
}
};
11 changes: 10 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"lib/*.js"
],
"dependencies": {
"setimmediate": "^1.0.5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidlehn -- can you speak to this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore the dependency, it's needed. See the MessageDigest-browser.js comments.

"fast-text-encoding": "^1.0.3",
"isomorphic-webcrypto": "^2.3.8"
},
"devDependencies": {
"benchmark": "^2.1.4",
Expand Down Expand Up @@ -62,6 +63,14 @@
},
"browser": {
"./lib/MessageDigest.js": "./lib/MessageDigest-browser.js",
"./lib/MessageDigest-reactnative.js": false,
"fast-text-encoding": false,
"isomorphic-webcrypto": false,
"rdf-canonize-native": false
},
"react-native": {
"./lib/MessageDigest.js": "./lib/MessageDigest-reactnative.js",
"./lib/MessageDigest-browser.js": false,
Comment on lines +66 to +73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simpler. No need to mask out deps that will never be required. The rdf-canonize-native one is needed since that is speculatively loaded. (And that code pattern could be avoided too.)

Suggested change
"./lib/MessageDigest-reactnative.js": false,
"fast-text-encoding": false,
"isomorphic-webcrypto": false,
"rdf-canonize-native": false
},
"react-native": {
"./lib/MessageDigest.js": "./lib/MessageDigest-reactnative.js",
"./lib/MessageDigest-browser.js": false,
"rdf-canonize-native": false
},
"react-native": {
"./lib/MessageDigest.js": "./lib/MessageDigest-reactnative.js",

"rdf-canonize-native": false
}
}
4 changes: 2 additions & 2 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ if(rdfCanonizeNative) {
}

const _TEST_SUITE_PATHS = [
program['testDir'],
program.testDir,
'../normalization/tests',
'./test-suites/normalization/tests',
];
Expand Down Expand Up @@ -195,7 +195,7 @@ function addTest(manifest, test) {
}

// expand @id and input base
const test_id = test['@id'] || test['id'];
const test_id = test['@id'] || test.id;
test['@id'] = manifest.baseIri + basename(manifest.filename) + test_id;
test.base = manifest.baseIri + test.input;
test.manifest = manifest;
Expand Down