Moved @aws-sdk/types to peer dependencies to resolve incompatibilities

[willarmiros]

Issue #, if available:
#439

Description of changes:
As title, based off customer's comment and AWS SDK recommendation. This way, we will be able to use whatever version of @aws-sdk/types is provided by the customer, which should resolve the error reported in #439. Tested on a sample app to verify instrumentation still works.

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

[willarmiros]

/cc @alexforsyth from AWS SDK V3

[codecov-commenter]

Codecov Report

Merging #449 (56d9d97) into master (73e6ba8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #449   +/-   ##
=======================================
  Coverage   82.48%   82.48%           
=======================================
  Files          36       36           
  Lines        1741     1741           
=======================================
  Hits         1436     1436           
  Misses        305      305           

---

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 73e6ba8...56d9d97. Read the comment docs.

[willarmiros]

Actually, after testing this with verdaccio, I realized this change will cause an issue similar to #411 in that users of this SDK who do not use the AWS SDK V3 (and therefore do not have @aws-sdk/types package available) will fail to compile. Therefore to maintain functionality for all users of this SDK, we must keep the types package as a proper dependency.

[m-radzikowski]

I think that with npm v6 and yarn, the non-installed peer dependencies are just logged as a warning during the install. And I see the npm v7 installs them by default now (https://stackoverflow.com/questions/35207380/how-to-install-npm-peer-dependencies-automatically).

I recall AWS SDK or some other libs have peer dependency on some frontend stuff for react etc., that is not needed nor installed when developing for Node.JS.

Let me check how it behaves on my own as well.

[willarmiros]

That is interesting that npm v7 installs them by default - sounds like that will solve this long-term but I think many people are still using npm 6 or less. It does in fact work with native JS (as long as you don't call the captureAWSV3Client API, which you wouldn't if you don't have an AWS SDK V3 dependency). Where it fails for me is running tsc on a project that uses aws-xray-sdk-core and does not satisfy the @aws-sdk/types dependency.

[m-radzikowski]

Ok, I reproduced the same on my own, and I see the problem.

I hoped that this: https://stackoverflow.com/a/63905376/2512304 can be the solution, but a quick test with changing the

import { Client } from '@aws-sdk/types';

to

import type { Client } from '@aws-sdk/types';

still results in a tsc error when @aws-sdk/types is not installed (in the client module that uses aws-xray-sdk-core). I either did something wrong or it's not a solution for this problem (again, only did a quick test).

I see that for the AWS SDK v2 you ignore the type completely: https://github.com/willarmiros/aws-xray-sdk-node/blob/peer-types/packages/core/lib/patchers/aws_p.d.ts#L7

While it would be best to have the captureAWSv3Client() fully typed, maybe it's better to not use @aws-sdk/types at all and do a similar signature here?

export declare function captureAWSClient<T>(client: T, manualSegment?: SegmentLike): T

As I see it:

• right now, the client type is checked only if I match the same @aws-sdk/types version as the xray lib uses,
• otherwise, I have to disable type checking on my own (with client as any).

Since you won't be releasing a new xray lib version every time there is a new AWS SDK v3 release, keeping @aws-sdk/types in sync would require using the old AWS SDK v3 lib version (that xray lib uses) and never update. In practice, I guess everyone will have to use as any. So from my perspective, it's far better to release the type restriction on the xray lib itself, getting rid of @aws-sdk/types completely here and making the function signature similar to the one for AWS SDK v2.

Well, it's my opinion and I can be wrong here, so please let me know what you think. It just hurts me when I have to do captureAWSv3Client(client as any) in my code 😄

[thesuavehog]

@willarmiros Can there be a decision made about how to deal with this issue? Either kill this PR so it doesn't appear in searches as a possible solution, or make a PR that addresses the problem by removing types. I have trained up 5 developers on using XRay now and every single one of them would have run into this issue and been confused if I hadn't built my own wrapper library around XRay to specifically as any (and then I have to explain to them why they have to use my library and ignore all the online documentation about how to wrap AWS SDK clients in XRay).

It seems like a pretty major issue that capture simply does not work per all the documentation about how to use it for a (growing) number AWS SDK v3 clients.

[NathanielRN]

Just some passing comments.

[NathanielRN]

Suggested change

	"//": "@types/cls-hooked is exposed in API so must be in dependencies, not devDependencies",
	"peerDependencies": {
	"@aws-sdk/types": "^3.0.0"
	},
	"peerDependencies": {
	"@aws-sdk/types": "^3.0.0"
	},
	"//": "@types/cls-hooked is exposed in API so must be in dependencies, not devDependencies",

[NathanielRN]

Did you mean ^3.0.0 to maximize compatibility?

Suggested change

	"@aws-sdk/types": "^3.18.0",
	"@aws-sdk/types": "^3.0.0",

[NathanielRN]

@thesuavehog sorry for your frustration with this issue. I'll add it to the agenda for our team to discuss next week.

In the meantime have you considered using the AWS Distro for OpenTelemetry (ADOT) - JavaScript solution?

OTel JS gets a lot of support from many devs and we announced GA support for it last year. All components are upstream and ready for you to use to trace with OTel JS and AWS X-Ray 🙂 We've been recommending users check that out so maybe you'll find excellent instrumentation there with minimal changes needed to get it working?

Let me know if you have any questions!

[willarmiros]

Superseded by #575.