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

fix(amazon-cognito-identity-js): set crypto for Node #7136

Merged
merged 3 commits into from
Nov 6, 2020

Conversation

amhinson
Copy link
Contributor

@amhinson amhinson commented Nov 6, 2020

Issue #, if available:
#7098

Description of changes:
This allows amazon-cognito-identity-js to run in the Node environment without having to explicitly add global.crypto = require('crypto') in your code.

I tested this in React Native, Expo, React, & Node. Normally, using a dynamic require like this causes problems with React Native, but since this only exists in cryptoSecureRandomInt.js and not cryptoSecureRandomInt.native.js, Metro bundler doesn't evaluate this particular file.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@amhinson amhinson changed the title Identity js node fix(amazon-cognito-identity-js): set crypto for Node Nov 6, 2020
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #7136 (020fbd7) into main (8b3183f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7136   +/-   ##
=======================================
  Coverage   72.95%   72.95%           
=======================================
  Files         213      213           
  Lines       13302    13302           
  Branches     2606     2510   -96     
=======================================
  Hits         9705     9705           
- Misses       3401     3432   +31     
+ Partials      196      165   -31     
Impacted Files Coverage Δ
packages/auth/src/OAuth/OAuth.ts 56.11% <0.00%> (ø)
packages/core/src/Credentials.ts 31.51% <0.00%> (ø)
packages/analytics/src/Analytics.ts 63.58% <0.00%> (ø)
packages/datastore/src/sync/index.ts 14.53% <0.00%> (ø)
packages/datastore/src/sync/outbox.ts 24.00% <0.00%> (ø)
packages/datastore/src/storage/storage.ts 71.66% <0.00%> (ø)
packages/core/src/OAuthHelper/GoogleOAuth.ts 33.33% <0.00%> (ø)
packages/core/src/Util/Reachability.native.ts 37.50% <0.00%> (ø)
packages/datastore/src/datastore/datastore.ts 78.42% <0.00%> (ø)
packages/xr/src/Providers/SumerianProvider.ts 47.55% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b3183f...020fbd7. Read the comment docs.

Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM 🌮 NIT: can we add an additional test for this?

Comment on lines +39 to +41
externals: {
crypto: 'crypto',
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for the build:umd to successfully build due to require('crypto'). However, I'm not even sure if the UMD build is even used anymore since webpack outputs the build to dist and we have "main": "lib/index.js" in package.json. With that said, removing the UMD build is out of scope for this PR.

Comment on lines +18 to +24
// Native crypto import via require (NodeJS)
if (!crypto && typeof require === 'function') {
try {
crypto = require('crypto');
} catch (err) {}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is based on how crypto-js handles this case: https://github.com/brix/crypto-js/blob/develop/src/core.js#L25-L30

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🌮 🎉 🥇

Thanks @amhinson

@amhinson amhinson merged commit 5173a99 into aws-amplify:main Nov 6, 2020
CryogenicPlanet pushed a commit to MLH-Fellowship/amplify-js that referenced this pull request Jan 20, 2021
* Set crypto for node

* add crypto to webpack externals

* add unit tests for crypto
@sanyashvets
Copy link

guys, why crypto instead of crypto-js ?

@github-actions
Copy link

github-actions bot commented Mar 2, 2022

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants