Add TypeScript definitions

[esilkensen]

Issue #, if available: #14

Description of changes:

This PR makes a complete first pass at adding TypeScript definitions for aws-xray-sdk. It leverages some of the work done in a previous PR that seems to be abandoned (#109, thank you to that author!)

I realize it's a pretty big PR, so I will be happy to do anything I can that might make it easier to review, and of course to iterate on it in response to any feedback.

It adds .d.ts files next to the corresponding .js files, and a "types": "lib/index.d.ts" entry to each package.json.

For testing, it uses the tsd tool which by default looks for TypeScript files to check in a test-d directory; each package has a test-d/index.test-d.ts file which exercises its TypeScript definitions, and the test script has been updated to run tsd as well.

Some initial questions I have:

• Is it exporting types that aren't necessarily meant to be part of the public API?
• How best to manage dependencies on @types packages for express/mysql/postgres?

---

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

[mmmeff]

Just curious - what's the reasoning for not adding these types to the @types/* monorepo? That's more of the standard approach in the typescript community as it allows the entire community to maintain type definitions without imposing maintenance of those types on a project's maintainers if they prefer plain js.

By checking these type definitions into this project, aws-xray-sdk now needs to maintain them as well, either directly or by repeatedly taking these kinds of PRs.

[esilkensen]

Hi @mmmeff,

I am certainly happy to create DefinitelyTyped PR's for the aws-xray-sdk packages instead if that's the better option. But if the maintainers are open to it, I think having the type definitions right next to the source makes it easier to check: "do these types align with the implementation?"

Potentially they could be updated along with JS changes to the public API, even in the same PR, without requiring a separate PR and review process in DefinitelyTyped. Then a TypeScript user can simply install this package and (hopefully) trust that the types that come along with it are in sync with the source.

For what it's worth, aws-sdk had a @types/aws-sdk package until version 2.7.0, and since then has been including types with aws-sdk itself -- though it is able to automatically generate some of them (maybe there's even something along those lines that could be built here too?)

[ArturGrigio]

@willarmiros

[willarmiros]

To answer your questions:

• The exported types seem to be fine as is, they line up with the exported functions and fields in the original package as far as I can tell. Were there any types in particular you were referring to?
• Having the @Types for express etc as dependencies is fine. We're exporting types that are defined in those external packages, so it'll be necessary to have them as dependencies for consumers of aws-xray-sdk.

Moving forward, we will be supporting TypeScript definitions in future releases of the SDK. The only foreseeable issue is that when we separate the AWS SDK dependency from the core module (item 4 in #157) we'll have to remove all TS definitions imported from aws-sdk. They'll probably just be replaced with any, which will degrade the usefulness of TS definitions in those places unless you see any ways around this.

As a final note, I am new to TypeScript (as you could tell by my reliance on @ArturGrigio to review this). I'd appreciate if the community point out any misses on my end as I make changes to TS definitions during future maintenance.

[esilkensen]

Thank you both @willarmiros and @ArturGrigio for reviewing - I've just merged the latest in from 2.x and added that segment.setUser() operation to segment.d.ts, and also updated the setServiceData type following our discussion.

As far as that first question I had, the types I was wondering about were mainly the properties on the sub/segment classes - whether they were public or private. So if things seem fine as they are now, that sounds good to me!

[esilkensen]

Here's the first of two references to aws-sdk - it could become res: any, no problem.

[esilkensen]

This aws_p.d.ts file has the only other reference to aws-sdk - it patches the AWS.Service classes in the return value of captureAWS and captureAWSClient so their request methods take an additional XRaySegment parameter.

After the dependency on aws-sdk is removed, I think in the worst case these functions could be generic identities that take any type T and return it.

Potentially, the referenced AWS type definitions (AWSError, Request, Service) could even be copied into this package and I think the types could stay about as they are now, thanks to TypeScript's structural type system, though maybe questionable whether that'd be worth it / a good idea...

[willarmiros]

Copying the definitions over is a possibility, I will get in touch with the AWS SDK team to see what they recommend.

[willarmiros]

Review has been completed, merging!

[kwongkz]

May i know when this will release to npm?

[willarmiros]

It will be included in the 2.5.0 release of the SDK. I cannot give an exact date but I will update this when it is released.

[willarmiros]

@kwongkz v2.5.0 has been released to NPM.

[esilkensen]

Awesome, thanks @willarmiros!

I was just looking again, should the segment argument to AWSXRay.resolveSegment be optional (i.e. for automatic mode)? Sorry, I think it may be something I missed in my PR, and the type lists it as required.

[willarmiros]

I guess that was missed on our end as well. Hmm this is where my TS knowledge is lacking. It is always invoked with an argument in the segment slot, but when operating in automatic mode, null is passed in instead of a segment object. It looks like whether TS will complain depends on whether the --strictNullChecks option is used?

Regardless, it should definitely allow for null values, and also be optional in case any customers call it without args.

[esilkensen]

As it is now, my understanding is that

• resolveSegment(undefined)
• resolveSegment(null)

will be accepted if --structNullChecks is false, but resolveSegment() will not; if --strictNullChecks is true, then they are all errors.

If the type is segment?: Segment | Subsegment | null, then they're all OK, regardless of --strictNullChecks, which I think is what we want?

Would you accept 2.x patches for this sort of thing? If so I'm happy to open a PR.

[willarmiros]

That makes sense. I agree that we want your proposal, and I'd accept a PR but our release process is quite cumbersome so it might only end up getting released as 3.0. If it's clearly a large pain for customers, I'll also release it as 2.5.1.

[esilkensen]

Sounds good, thanks -- yeah this change doesn't feel like it warrants a new release by itself. I'll open a PR!

[LosD]

(moved from the issue, as this seems like a better place. If you want me to open a proper issue, I will. But I think it's just a question for now)

@willarmiros @esilkensen Is it on purpose that @types/continuation-local-storage has been added to dependencies instead of dev-dependencies? That seems like a mistake, as it'll make the package larger (right now, that of course drowns in comparison to the aws-sdk, but when that is removed, the typings is the largest dependency by far)

[esilkensen]

Hey @LosD, since the @types/cls-hooked package (or @types/continuation-local-storage as it is now) is referenced as part of this package's public API, TypeScript users will have to download those types one way or the other, otherwise the compiler won't be able to resolve that import { Namespace } from 'cls-hooked'; statement in context_utils.d.ts.

I think it's most convenient as a dependency, but it could be listed as a peerDependency, prompting all users of the package to install the typings separately, which may not be ideal for non-TypeScript users?

Anyway, it looks like the @types/cls-hooked and @types/continuation-local-storage packages are only about 4KB each -- maybe you were looking at the size of the continuation-local-storage dependency itself (100KB)?

[LosD]

@esilkensen Hmmm, you may be right, I mainly do application development, so maybe library development HAS to do it this way, and rely on tree-shaking not to bring it to runtime (something we currently can't do in our project, because annoying reasons 🤦‍♂️).

However, while the size of @types/continuation-local-storage in itself is only 5.4k, it draws in @types/node, making the total size more than 670k (before compression, of course):

edit: from package-lock.json:

    "@types/continuation-local-storage": {
      "version": "3.2.2",
      "resolved": "https://registry.npmjs.org/@types/continuation-local-storage/-/continuation-local-storage-3.2.2.tgz",
      "integrity": "sha512-aItm+aYPJ4rT1cHmAxO+OdWjSviQ9iB5UKb5f0Uvgln0N4hS2mcDodHtPiqicYBXViUYhqyBjhA5uyOcT+S34Q==",
      "requires": {
        "@types/node": "*"
      }
    }

[LosD]

Just read TypeScript's Publishing document, and they recommend doing it exactly as you are doing it, so it's just me missing finer points in library publishing. 😊

I'll try to find out why it ends up in my runtime dependencies (other unused modules doesn't seem to)

[esilkensen]

Ah, thanks, good call -- yes, and@types/cls-hooked has the same dependency on @types/node.

Well, I'm interested what others think about this, too...

Since this package is only referencing that single Namespace interface from the @types dependency, it seems like if the size is prohibitive it might not be an issue to even remove the dependency altogether and just mark the return type with any or some approximation of the real interface.

[esilkensen]

Or going back to your initial point about not needing the types at runtime, maybe it gets to (like others mentioned up above) whether we do want the types packaged with this library or if they're better off as a separate @types dependency, because of the package size. In any case, if there is anything I can do to help out I will be happy to.

[LosD]

For me, I kinda like that it's integrated; It shows that the project takes responsibility (though in some cases, that's a promise that's not kept), but if it packing developer-only stuff into users' runtimes, that might be a good reason to avoid it. But then again, it is probably me that needs to get the project packaging fixed.