Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamo DocumentClient is not exposed in TypeScript definitions #1233

Closed
midknight41 opened this issue Nov 18, 2016 · 23 comments · Fixed by #1240
Closed

Dynamo DocumentClient is not exposed in TypeScript definitions #1233

midknight41 opened this issue Nov 18, 2016 · 23 comments · Fixed by #1240

Comments

@midknight41
Copy link

The DocumentClient class isn't exposed via the TypeScript definitions. I believe the following should work but it does not.

const client: AWS.DynamoDB.DocumentClient = new AWS.DynamoDB.DocumentClient();

I notice you do some code generation with typescript files so I thought I'd check in with you before doing a PR.

@chrisradek
Copy link
Contributor

@midknight41
You're right. If you don't explicitly define client as DynamoDB.DocumentClient, the typescript compiler is able to infer the correct type, but it isn't exposed in a way for you to explicitly define it.

You're welcome to submit a PR, but I think this falls under the generated typings so it is something we'd want to tackle in a generic way (we likely have the same problem for S3.ManagedUpload as well.

@midknight41
Copy link
Author

There are two issues here actually. I've defined it badly.

  1. You cannot explicitly state the variable type
  2. The AWS.DynamoDB namespace does not contain a DocumentClient class. new AWS.DynamoDB.DocumentClient() gives a error.

I'll have a peek at the other issue first I think.

@sgtoj
Copy link

sgtoj commented Nov 18, 2016

Other semi related problems are some interfaces are not exported:

  • AWS.DynamoDB.PutParam
  • AWS.DynamoDB.GetParam
  • AWS.DynamoDB.UpdateParam
  • AWS.DynamoDB.QueryParam
  • AWS.DynamoDB.ScanParam

@chrisradek
Copy link
Contributor

@sgtoj
Are you referring to the parameters for DynamoDB.DocumentClient? You should be able to access the interfaces on AWS.DynamoDB.Types.

@sgtoj
Copy link

sgtoj commented Nov 18, 2016

Thanks... You're correct. I was using @types/aws-sdk before types definition were added (v2.7.0?) to the package. Those definitions from DefinitelyType has it defined differently.

@midknight41
Copy link
Author

There is quite a lot of variation between the DefinitelyTyped ones and the ones AWS are producing. There will be lots of breaking changes but, ultimately, the definitions here will be a lot better and maintained better.

@sgtoj
Copy link

sgtoj commented Nov 18, 2016

That is my hope. I wasn't aware of the packaged types so I used the DefinitelyTyped one. That is until today. I will be using the native definition from this point forward.

@MZhoume
Copy link

MZhoume commented Nov 19, 2016

@midknight41 Same issue here. Any updates?

@dinvlad
Copy link

dinvlad commented Nov 19, 2016

So how do we deal with this issue at the moment? Thanks

FWIW DynamoDB.Types also doesn't seem to be exported, at least in 2.7.5

@chrisradek
Copy link
Contributor

@midknight41 and others.
Regarding point #2, I am able to create a new AWS.DynamoDB.DocumentClient without any TypeScript compiler errors, I just can't explicitly tell a variable that it should be that type (you first point). Is that not the case for others in here?

@dinvlad
Copy link

dinvlad commented Nov 19, 2016

Ok, that's right -

const db = new DynamoDB.DocumentClient();

seems to work for me as well. At the same time,

Module 'DynamoDB' has no exported member 'DocumentClient'
Module 'DynamoDB.Types' has no exported member 'DocumentClient'

So the workaround for now if we want to pass this object around, is to pass it as 'any' type. Thanks

@midknight41
Copy link
Author

@chrisradek In regards to the DynamoDB.DocumentClient class, I was getting compile errors but I had a few things wrong with my setup.

I'm tidying thing up at present. I'll feedback once I've done that.

@midknight41
Copy link
Author

@chrisradek the new AWS.DynamoDB.DocumentClient error was a red herring. Ignore that one.

With regard to the fixing the first issue: What is the expectation in terms of the namespaces?

For the DocumentClient is it:

const client: AWS.DynamoDB.DocumentClient;

or

const client: AWS.DynamoDB.Types.DocumentClient;

and for the Types exposed for DocumentClient is it:

const params: AWS.DynamoDB.DocumentClient.Types.GetItemInput;

or

const params: AWS.DynamoDB.DocumentClient.GetItemInput;

or something else? I think they can't be in the same namespace as the mainDynamoDB.Types namespace as I believe there are some naming collisions.

I have to admit, I find the Types namespace a bit counter-intuitive but there you go.

If I'm to help with this I'll need to understand which files are generated and which ones are maintained by hand. Could you enlighten me?

@chrisradek
Copy link
Contributor

@midknight41
I took a look at exposing DocumentClient (as well as other classes like ManagedUpload on S3) this weekend. I've updated the typings generator to export these classes and their interfaces (see #1240)

I still need to export the typings for params/output for each operation these classes use. The only way to do this that I can see is to export them in the generated files as well, like the other interfaces that these classes expose: https://github.com/aws/aws-sdk-js/pull/1240/files#diff-78251b595701d9bf6bbe21acda93b196R146

I don't like having to declare a namespace alongside a class (i.e. class DynamoDB and namespace DynamoDB) just to expose these interfaces since it leads to a lot of duplication, but I haven't found a better way. If you know a better way I'm all ears!

@midknight41
Copy link
Author

@chrisradek I'll have a look over this on the way home and see if I can suggest anything better.

@midknight41
Copy link
Author

Hey @chrisradek,

In terms of minimising the amount of code, combining the type definitions for DynamoDB and DynamoDB.DocumentClient into a single file yields the most benefit I think. The DocumentClient API is only slightly different that the main DynamoDB API and their is tonnes of duplication between the DynamoDB.DocumentClient.Types namespace and the DynamoDB.Types namespace.

With both namespaces in the same file you avoid mapping every type from the DCInterfaces.d.ts file to the namespace declared in the main client\dynamo.d.ts file.

I've hacked a version of that together locally which I can put up somewhere if you'd like to see it.

@chrisradek
Copy link
Contributor

@midknight41
Sure, I'd like to take a look at your local version of you don't mind sharing. DynamoDB is a unique case compared to the other classes that live on service client namespaces, since it shares so many interfaces. I think you may be right that this is the better route to go.

@midknight41
Copy link
Author

@chrisradek

Here's the PoC as discussed:
master...midknight41:master

Here's the test file:
https://github.com/midknight41/aws-sdk-js/blob/master/ts/dynamodb.ts

I've not eliminated all the duplicates but it's easy to see where you can chop out lots of duplicate declarations. Might be a tidy solution with a generic to cut them down a bit more...not quite sure without looking at it more.

Hope that helps.

@uxsoft
Copy link

uxsoft commented Nov 30, 2016

I can't wait for this to be ready! :-)

@chrisradek
Copy link
Contributor

@midknight41
Thanks for the file! I should have some time the rest of this week to take a look at this and try tackling the remaining objectives.

@chrisradek
Copy link
Contributor

I've updated the PR. Thanks @midknight41 for your help, I got some inspiration from your diff :)

The latest changes in the PR exposes classes that live on service clients, and their interfaces.

I made a change so that you can reference interfaces without the Types namespace, though Types is still there for backwards compatibility.

For example, you can do the following:

import DynamoDB = require('../clients/dynamodb');
// define the 'DocumentClient' type
const client: DynamoDB.DocumentClient = new DynamoDB.DocumentClient();

// define the DocumentClient.GetItemInput type directly off the DocumentClient
const params: DynamoDB.DocumentClient.GetItemInput = {
    TableName: 'MyTable',
    Key: {
        'my-key': 'value'
    }
};
// reference service interfaces directly off the service namespace
const getParams: DynamoDB.GetItemInput = {
    TableName: 'MyTable',
    Key: {
        'my-key': {
            S: 'value'
        }
    }
};

I've started writing tests, but I need to write some more for S3.ManagedUpload and CloudFront.Signer still. Will also need to update this to support Polly.Presigner, but we're almost there!

@midknight41
Copy link
Author

Looks really good!

@lock
Copy link

lock bot commented Sep 28, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants