amplify-js

amplify-js copied to clipboard

Description of changes

Upgrade uuid to avoid some annoying install-time warnings about potential risks in older uuid's (#8464). Trying this again after previous attempt was rolled back (#9159)

Issue #, if available

https://github.com/aws-amplify/amplify-js/issues/8464

Description of how you validated changes

ran tests on CI

Checklist

• [x] PR description included
• [x] yarn test passes

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

@MatrixFrog we had to revert ur earlier upgrades of uuid because of failure in RN. If you could test it out in RN and let us know how it goes, we could merge this. Or we will investigate this issue in an upcoming cycle and post on how the investigation goes.

RN meaning react-native? Is there a little sample react-native project I can test on or something?

@MatrixFrog I will get back on this today. there seems to be an issue in react native in datastore usage in specific. Let me get more detail and ping here.

I think I found the error you're talking about:

Error: crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported

Will try to look into it further tomorrow.

so i guess the easiest might be to just tell users they need to add import 'react-native-get-random-values'; before importing the datastore package, which presumably would be a breaking change. kind of annoying react-native doesn't just ship with an implementation of crypto that matches other environments :(

This is the same approach taken here: https://github.com/aws/aws-sdk-js-v3/blob/main/README.md#getting-started what do you think?

Is there a way to just to make the polyfill a peerDependency and include it if crypto.getRandomValues() doesn't exist? This way it is transparent to developers. It might not even be a breaking change? At least it's not clear why it would be...

If it is a breaking change, is there a way to put it behind a feature or version flag...so that new projects, and projects not using datastore can move forward...

Any plan to resolve conflicts and release the fix?

Might be easier to switch to a different uuid generator...react-native-uuid or something?

I'm happy to resolve conflicts if it will help get this merged!

Hi @MatrixFrog - this change would break existing React Native customers. So we will be holding off on merging it until our next major version bump.

so i guess the easiest might be to just tell users they need to add import 'react-native-get-random-values'; before importing the datastore package

is there some kind of release notes file I should add this to?

@MatrixFrog - this would actually make more sense to add to our documentation on docs.amplify.aws. We can handle that once the PR is ready to be merged as part of our next major version bump. We do not have an exact ETA on it, but we are tracking this PR as a part of that effort.

@MatrixFrog, this upgrade to the uuid version is now out in v6 of Amplify. Can you update and validate if the concerns in this PR are addressed?

looks like all the places that a package depends on uuid are now at ^9.0.0

thanks!