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

Jump to bottom

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 2 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
8 changes: 2 additions & 6 deletions lib/MessageDigest-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@
*/
'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
const crypto = require('isomorphic-webcrypto');
Copy link
Member

Choose a reason for hiding this comment

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

How much does this bloat the package size? Is there a way for us to only load this for react-native? We had considerable size savings by just using native implementations before. Shouldn't react-native apps just pull this in on their own and expose it as a global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always a good question (re package size). Far as I can tell: https://bundlephobia.com/result?p=isomorphic-webcrypto@2.3.8 (10.5k minified, 3.7k gzip'd).

Pulling in crypto as a global, as a technique, is not a bad idea, worth exploring.

(Also, I very much wish that React Native had a similar section that the "browser" has in package.json. As far as I can tell, it just uses the "browser" section, actually. Need to investigate.)

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 you know what, RN straight up has a "react native" section of package.json, that acts exactly like the browser section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlongley should be fixed now.


module.exports = class 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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"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.

"isomorphic-webcrypto": "^2.3.8"
},
"devDependencies": {
"benchmark": "^2.1.4",
Expand Down
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