SDK pulls in development level dependencies for production

[Carsten-Leue]

This SDK is used by several other IBM cloud SDKs, in my case the @ibm-cloud/secrets-manager SDK. My application is a consumer of the @ibm-cloud/secrets-manager and I would like to package it (and its dependencies) as part of my app.

I notice that this causes a significant amount dependencies to be pulled in that are not required for production and that should also not appear in the dependencies of a production level application, e.g. the expect package. This package is meant to write unit tests and should only be a development level dependency. It alone adds around 200KB to the overall bundle size, because it in turn depends on jest packages which should not exist in a production environment either (see https://bundlephobia.com/package/expect@27.3.1)

The reason for this dependency is the fact that this core SDK exports utility functions for testing purposes as part of its regular index (https://github.com/IBM/node-sdk-core/blob/main/index.ts#L21)
Since this SDK is also only available as a commonjs package, there is also no way to use tree shaking to get rid of these undesirable and unused dependencies.

I suggest to move the test functions to a sub-package with its own package.json and its own dependencies. This way consuming SDKs can still make use of the exported test helpers but they would not interfer with the production usage of the package.

Besides the undesirable bundle size this also effects legal clearance when we at IBM depend on the SDK, because test level packages typically do not have to be cleared with the same diligence as production level packages. With the current design, we are forced to clear test packages.

[padamstx]

@Carsten-Leue Thank you for the issue. The SDK squad will look into it.

[Carsten-Leue]

This is e.g. the size of the expect dependency:

[padamstx]

I suggest to move the test functions to a sub-package with its own package.json and its own dependencies. This way consuming SDKs can still make use of the exported test helpers but they would not interfer with the production usage of the package.

Would you have some advice on how to achieve this packaging change in the node core repo?
Are you suggesting we move to a monorepo where we have one package that is the core production code and another with the test helpers? One of the challenges that we would potentially have is the fact that we use the semantic-release tool to automate our release management based on commit messages, etc. Plus by splitting out the test helpers into a separate package, that would impact existing SDK projects that currently depend on the node core, although perhaps we could do the package-splitting and create a new major version of the node core so that SDK projects would need to consciously move up to the new version.

cc: @dpopp07

[CarstenLeue]

@padamstx @dpopp07 I suggest to use the concept of a "secondary entry" point to expose the test level dependencies. You won't need to change the structure of the project, just adding an extra package.json to a subfolder (i.e. the secondary entry point) and removing the exports from top level is enough. This concept of secondary entry points is very popular in the Angular community but in no way restricted to Angular. But you find most of the related doc in ng docs, e.g. https://medium.com/tunaiku-tech/creating-secondary-entry-points-for-your-angular-library-1d5c0e95600a or https://github.com/ng-packagr/ng-packagr/blob/master/docs/secondary-entrypoints.md

Yes, this changes the public API of the SDK if you also remove the top level export. One approach would be to deprecate the top level exports (but leave them in) and introduce the secondary entry point in a minor version change. This way clients can move over. Together with the tree shaking PR they would still benefit from reduced dependencies.

In such an effort I recommend to revisit if the actual API the SDK exposes is indeed the intended API (I am sorry if this is sounding arrogant, but in node it's probably not evident what's exposed as the API at first glance. If the actual API actually matches the intended API, please ignore the following).

The exposed API is this:
ibm-cloud-sdk-core.api.md

Note that dependencies of this API are not the same as implementation dependencies.

You might want to consider this:

• the API has a dependency on node interfaces. If the SDK is meant to be used in the browser you might want to reconsider exposing nodejs APIs as part of the SDK API.

import { OutgoingHttpHeaders } from 'http';
import { Stream } from 'stream'

• the API is not complete, e.g. AuthenticateOptions is not exported but used in the API
• method such as atLeastOne or atMostOne look like utility functions. Do they have to be part of the API?

[mpawlow]

• I second all of the concerns outlined in the issue description
• An additional concern is that the expect dependency (which should be classified as a dev dependency) is being included (as a production dependency) in OSS scans and is currently flagged with a security vulnerability that needs to be resolved
  • Commented accordingly in the related issue: move some dependencies to devdependencies #266 (comment)

[s100]

+1 for this. It's extremely strange for a production library to list unit testing utilities like expect as production dependencies. expect indirectly pulls in things like chalk, @jest/expect-utils, micromatch, braces and @babel/highlight - a total of around 3.6MB of additional code. On top of everything else, this adds to the security vulnerability attack surface, as seen in the recent CVEs against micromatch and braces.

[dpopp07]

Thanks all for the comments - agreed. We will address this in the very near future.

[ibm-devx-sdk]

🎉 This issue has been resolved in version 4.3.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[ibm-devx-sdk]

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀