Trim production dependencies

[mhart]

Is your feature request related to a problem? Please describe.

I'm hoping to switch to aws-sdk-js-v3 for its modularity, in the hope that the package sizes are smaller than v2, but I notice a lot of dependencies being installed that don't seem necessary at runtime, which actually make it quite a large npm install.

For example, middleware-retry has as production dependencies:

• react-native-get-random-values

This only seems to be needed for very specific environments (react native) and so users who need it should install it as a peer dependency, but to have everyone install it (and therefore have to maintain it, and patch it if there are security issues, etc) seems an overreach.

- `@aws-sdk/types`

This seems to a module made up of TS and empty *.js files. My guess is it should be a dev dependency only for typescript, and possibly reworked so the js files aren't included at all.

(addressed in #1649)

- `tslib`

This should only be used at dev time too – for bundling. I doubt that this is used during runtime (but could be wrong?)

Describe the solution you'd like

Ensure that each module only has the very minimum that it needs in terms of production (non-dev) dependencies.

Additional context

One HUGE advantage of aws-sdk v2 as it stands right now is that it has very few dependencies – this means that users don't end up having to constantly patch sub-sub-dependencies of modules they don't even use. It would be great if v3 could also carry this philosophy over, as well as its goal of providing small individual modules for tree shaking

Right now I'm quite surprised at the size. Eg, installing just a single client, eg @aws-sdk/client-ssm, results in 102 packages being installed (half of which seem to be non-AWS modules) and a 19.2MB node_modules. That's almost half the size of the entire aws-sdk v2 for just one client – and a lot more packages to potentially patch and maintain

[mhart]

Actually, apologies on the tslib front – I was assuming that you were bundling everything with webpack into distribution files, which would have made tslib just a dev dep, but it looks like tslib is needed for the individual files.

[mhart]

I'd love some feedback here – I keep seeing @aws-sdk/types being added as a production dependency to new modules being added to this project, and I really don't think it should be

[willfarrell]

@mhart well said.

I was looking into react-native-get-random-values, basically it add a function to global.crypto:

const base64Decode = require('fast-base64-decode')
const getRandomBase64 = require('react-native').NativeModules.RNGetRandomValues.getRandomBase64

class TypeMismatchError extends Error {}
class QuotaExceededError extends Error {}

global.crypto.getRandomValues = function (array) {
  if (!(array instanceof Int8Array || array instanceof Uint8Array || array instanceof Int16Array || array instanceof Uint16Array || array instanceof Int32Array || array instanceof Uint32Array || array instanceof Uint8ClampedArray)) {
    throw new TypeMismatchError('Expected an integer array')
  }

  if (array.byteLength > 65536) {
    throw new QuotaExceededError('Can only request a maximum of 65536 bytes')
  }

  base64Decode(getRandomBase64(array.byteLength), new Uint8Array(array.buffer, array.byteOffset, array.byteLength))

  return array
}

Where react-native is peerDependency ... is this function ever triggered? Cause it would fail if it was without react-native installed. Can we just remove this? Why is a dependency on react* all over this repository? Does this mean react* is installed on every nodejs lambda function that runs? If so, that would raise some concerns.

cc #1797

[G-Rath]

@mhart @aws-sdk/types is required as a "production" dependency for TypeScript users - the alternative is to have it as an optional peer dependency which'll means developers downstream will always have to install that package in addition themselves.

There are pros and cons to both, but someone "loses" either way - the fact that it's got empty JS files however is less optimal and in theory should be resolvable.

react-native-get-random-values however most definitely should be made an optional peer dependency using peerDependenciesMeta.optional (I've linked to the Yarn RFC, but it also applies to npm). I can't stress this enough as NPM7 has restored it's original behaviour of installing peerDependencies by default, meaning it's no longer enough to just put a dependency in peerDependencies (this is part of why #1797 is happening I think - right now npm 7 will install react-native because of this).

This will mean that users using react-native will have to install the dependency themselves, and that either the import will have to be done in developer space or the clients will have to import it with a try/catch.

[mhart]

@G-Rath @aws-sdk/types is only required for TypeScript users if they want type information – which is definitely not a runtime concern. You wouldn't install @aws-sdk/types in your dependencies – you'd install them in your devDependencies like all other type information

[G-Rath]

@mhart @aws-sdk/types is required for TypeScript users if the types are being used, making them a required dependency for the project, especially since it's a library not an app (in an app such packages 💯 should always be devDependencies).

The problem effectively boils down to that we currently have no way of marking a dependency as being required for compiling vs being required for actually running.

As I said above, there are two ways forward in lue of that: marking them as dependencies or as peerDependencies - this library has taken the dependencies route. They're not the first to do this nor will they be the last*, but the good news is that the package should be really small and not make it into your production builds if you've got basic tree-shaking in place :)

*I personally don't mind either route, since they both result in the same thing

[mhart]

The @aws-sdk/types issue was addressed in #1649 (comment)

[G-Rath]

FYI npm@7 has been made generally available, meaning people will be prompted to upgrade. This version of npm installs non-optional peerDependencies by default, meaning that react-native will be installed as a production level dependency when using the aws libraries.

Would be great to get this moved to be properly optional - happy to help with this if desired.

[jimfleming]

@G-Rath is correct. To revert back to the npm@6 behavior we can use npm install --legacy-peer-deps. This requires first deleting the package-lock.json and node_modules as it won't remove peer dependencies that were already installed.

Alternatively, we can explicitly omit peer dependencies: npm install --omit peer which will remove existing peer dependencies.

[tuarrep]

The react-native dependency is problematic as it introduces security vulnerability as it leads to install node-fetch.
This dependency has no fix as time of writing.

By investigating the middleware-retry package, I've found where react-native-get-random-values is used

aws-sdk-js-v3/packages/middleware-retry/src/index.native.ts

Line 2 in 5cb9c7b

import "react-native-get-random-values";

A comment on the previous line points out to https://github.com/uuidjs/uuid#getrandomvalues-not-supported. My understanding of this is:

• React Native doesn't support getRandomValues()
• react-native-get-random-values is a polyfill for it
• middleware-retry is importing it to be functional on React Native runtimes

In my understanding, we have two options here:

1. Adding react-native-get-random-values as optional peer dependency in middleware-retry package (https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependenciesmeta)
2. Opening a PR upstream to define react-native as optional peer dependency (https://github.com/LinusU/react-native-get-random-values/blob/3afc0fa7df76dc2bd32535c69eb4986575e77530/package.json#L10)

IMHO, it must be done upstream as the package is React Native specific but AWS SDK isn't. I mean I think peoples which explicitly require react-native-get-random-values have already installed React Native that is clearly not the case for AWS SDK.

See npm audit result below:

# npm audit report

node-fetch  <=2.6.0 || 3.0.0-beta.1 - 3.0.0-beta.8
Denial of Service - https://npmjs.com/advisories/1556
No fix available
node_modules/isomorphic-fetch/node_modules/node-fetch
  isomorphic-fetch  2.0.0 - 2.2.1
  Depends on vulnerable versions of node-fetch
  node_modules/isomorphic-fetch
    fbjs  0.7.0 - 1.0.0
    Depends on vulnerable versions of isomorphic-fetch
    node_modules/fbjs
      metro  0.22.1 - 0.63.0
      Depends on vulnerable versions of fbjs
      Depends on vulnerable versions of metro-config
      node_modules/metro
        @react-native-community/cli  *
        Depends on vulnerable versions of metro
        Depends on vulnerable versions of react-native
        node_modules/@react-native-community/cli
          react-native  <=0.0.0-6d6c68c2c || 0.0.0-722a4ba56 - 0.0.0-72ac2125b || 0.0.0-73242b45a - 0.0.0-851d01b0a || 0.0.0-87d000f89 - 0.0.0-bf0539fb5 || 0.0.0-c3ab2a254 || 0.0.0-c9f869f9c - 0.0.0-e661a551c || 0.0.0-e6fc20ee6 - 0.0.0-e8060ae10 || 0.0.0-e96f1e1d8 || 0.0.0-eab7fc008 - 0.0.0-ee3994530 || 0.0.0-f00795dc9 - 0.0.0-ffdfbbec0 || 0.22.0-rc - 0.64.0-rc.0
          Depends on vulnerable versions of @react-native-community/cli
          Depends on vulnerable versions of fbjs
          node_modules/react-native
            react-native-get-random-values  *
            Depends on vulnerable versions of react-native
            node_modules/react-native-get-random-values
              @aws-sdk/middleware-retry  >=1.0.0-gamma.2
              Depends on vulnerable versions of react-native-get-random-values
              node_modules/@aws-sdk/middleware-retry
                @aws-sdk/client-secrets-manager  >=1.0.0-gamma.3
                Depends on vulnerable versions of @aws-sdk/middleware-retry
                node_modules/@aws-sdk/client-secrets-manager
        metro-config  <=0.63.0
        Depends on vulnerable versions of metro
        node_modules/metro-config

10 low severity vulnerabilities

Some issues need review, and may require choosing
a different dependency.  

[mhart]

What @tuarrep says is exactly why I created this issue and noted:

to have everyone install it (and therefore have to maintain it, and patch it if there are security issues, etc) seems an overreach

[G-Rath]

IMHO, it must be done upstream as the package is React Native specific but AWS SDK isn't. I mean I think peoples which explicitly require react-native-get-random-values have already installed React Native that is clearly not the case for AWS SDK.

That is how it's done upstream: react-native-get-random-values depends on react-native (i.e it uses files from react-native so will not function without it) , so it correctly lists it as a required peer dependency.

The AWS SDK does not require react-native to function - it does require react-native-get-random-values if you're running it on react-native, so it should be listed as an optional peer dependency and conditionally require it at runtime.

@mhart I'm wondering if someone should just open a PR implementing this change, as the patch should be relatively small. I'd be happy to help support such a PR (I can't commit the time right now to doing the opening, otherwise I'd be happy to do that too 🙂)

[tuarrep]

I'm not a React Native expert and I haven't a setup to test neither I'm comfortable with this repository conventions (ES version support?)
Is the following sounds good to you?

if(!'getRandomValues' in crypto)
  import 'react-native-get-random-values';

I could blindly craft a PR if it can help.

[G-Rath]

@tuarrep You're wanting to do something like this:

try {
  require('react-native-get-random-values';
} catch {
 // do nothing 
}

• we use a try/catch because we're wanting to import the package if it's available but not care if its not available
• we use require because the ES equivalent for dynamic imports is the import() function, which is async
• we don't care if getRandomValues is actually defined in crypto because that's an implementation detail - all we care about is react-native-get-random-values provides support for something that's not in react-native natively, and so if that package is available we'll import it on behalf of the developer (because really, this is something that could be entirely done in developer land - this is actually just a nicety to save some typing)

We'd also want to add react-native-get-random-values as an optional peer dependency in the related package.json:

{
  // ...
  "peerDependencies": {
    "react-native-get-random-values": "^1.5.0"
  },
  "peerDependenciesMeta": {
    "react-native-get-random-values": {
      "optional": true
    }
  }
  // ...
}

[tuarrep]

Thanks, @G-Rath for your explanation, I like your approach.

I'll draft a PR tomorrow if no one else has done that before.

[trivikr]

Opening a PR upstream to define react-native as optional peer dependency (LinusU/react-native-get-random-values@3afc0fa/package.json#L10)

A request is created upstream to add react-native as an optional peerDependency LinusU/react-native-get-random-values#27

[G-Rath]

@trivikr except that it doesn't make sense for them to have react-native as an optional dependency when its required for the library to work.

Also, you can't just go around to every react-native polyfill library and ask them to make this change - all it takes it for one not to do it, and this is where you'll end up.

[trivikr]

Also, you can't just go around to every react-native polyfill library and ask them to make this change - all it takes it for one not to do it, and this is where you'll end up.

That's right. I've closed the issues with react-native polyfills repos:

• Make react-native peerDependency optional LinusU/react-native-get-random-values#27
• Make react-native peerDependency optional charpeni/react-native-url-polyfill#269

Decision taken by the team during post-scrum on 3/26: Make react-native polyfills as optional peer dependencies in aws-sdk-js-v3.

Question yet to be answered:

• Should aws-sdk-js-v3 call react-native polyfills in the code, or should they be delegated to consumers which consume aws-sdk-js-v3 in react-native environments? The team is inclined to delegate the requirement to consumers.
• If react-native polyfills are to be called from the code, should they be called from .native.ts files? This way, an error would be thrown in React Native environments is aws-sdk-js-v3 customer on react-native platform doesn't have polyfills installed.

[arciisine]

Just got hit by this as well, unexpectedly. I just looked thorugh all the middleware code, and I couldn't seem to find where the get random value code was even being used. Is this something that could be externalized as the responsibility of the library consumers to pull in, when targeting native environments? Or even a different package that wraps the client sdk, and includes this as part of the main entry point? It seems like the library is attempting to polyfill for native environments in a middleware library, which just seems odd.

[trivikr]

The react-native polyfills were removed in #2191

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.