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

DynamoDB Document Client/DAX Support #330

Merged
merged 8 commits into from
Jun 13, 2018
Merged

DynamoDB Document Client/DAX Support #330

merged 8 commits into from
Jun 13, 2018

Conversation

fishcharlie
Copy link
Member

@fishcharlie fishcharlie commented Apr 3, 2018

This PR adds support to set the DynamoDB document client, which can be used to use DAX with Dynamoose.

Close #212, #261.

@fishcharlie fishcharlie mentioned this pull request Apr 3, 2018
@coveralls
Copy link

coveralls commented Apr 22, 2018

Coverage Status

Coverage increased (+0.02%) to 82.511% when pulling c6db398 on fishcharlie:dynamodbclient into 388662c on automategreen:master.

@sirgalleto
Copy link

What is missing for the approval? May I help you with something?

@fishcharlie
Copy link
Member Author

@sirgalleto A second code review would be good. I'm pretty sure it's good, but would love some feedback if you think it's good or not or needs more adjustments.

Besides that I'm waiting for @brandongoode to reply to my emails. Once that happens I will finish up version 0.9, which this PR will be included in.

@sirgalleto
Copy link

I see. Currently, I was intensely looking into the documentation for this exact implementation, a way to set the client to use DAX which is the current blocker of my team and the reason for choosing another ORM as Dynogels. If I'd know the new version release plan, I'd have enough arguments for still using Dynamoose.

The documentation is added correctly, tests increasing the coverage and clean implementation. From my view, this is a high leverage PR due to the straightforward implementation to use DAX.

Thanks for this great contribution 👍

@fishcharlie
Copy link
Member Author

@sirgalleto I would love to be able to give a better timeframe for when this will be released in an NPM version. But if you asked me on April 3rd when I thought this would be available in a release version I would have for sure said before today. I'm hoping to get it integrated soon tho!!

@sirgalleto
Copy link

Thanks for your notification, I'll keep watching this PR 💯

@fishcharlie fishcharlie modified the milestones: v0.9.0, v1.0 Jun 13, 2018
# Conflicts:
#	dynamoose.d.ts
@fishcharlie fishcharlie merged commit 84b15eb into dynamoose:master Jun 13, 2018
@fishcharlie fishcharlie deleted the dynamodbclient branch June 13, 2018 01:33
@fishcharlie fishcharlie mentioned this pull request Jun 13, 2018
@fishcharlie fishcharlie restored the dynamodbclient branch June 13, 2018 01:46
@sirgalleto
Copy link

👏

@zhenyulin
Copy link

is there a timeline when this would be released?

@fishcharlie fishcharlie deleted the dynamodbclient branch July 18, 2018 16:31
@fishcharlie
Copy link
Member Author

@zhenyulin This is part of version 1.0 (#365). Once #376 gets merged in we can release version 1.0. Currently we have some failing tests on PR #376. Any help solving that would be much appreciated and help version 1.0 be released quicker.

@Nikhil2508
Copy link

Has DAX support been integrated? If yes, can someone please post the link to necessary documentation/example. Thanks

@fishcharlie
Copy link
Member Author

@Nikhil2508 Yes, it has been released as of Dynamoose version 1.0. This is the PR that has all the changes in it, including documentation changes. You can also find the documentation on the Dynamoose website.

@ThienNDDi
Copy link

@fishcharlie I using dynamoose V3.
I can not find setDocumentClient variable.
Is it changed to Instance?
Thank very much.

@fishcharlie
Copy link
Member Author

@ThienNDDi This is a 6 year old PR. Lot has changed since then. There is an active discussion in the Dynamoose Slack about this. There are a few complications with circular references and lack of promise support on AWS's end. You can follow along more there.

@ThienNDDi
Copy link

@fishcharlie Can you provide me with more detail? I checked slack but it's requiring i have to upgrade plans.
So if i wanna use DAX with dynamoose, have you any idea?

@fishcharlie
Copy link
Member Author

@ThienNDDi There shouldn't be any need to upgrade. I don't think DAX is currently working in Dynamoose is the short answer. Hopefully we can get it working soon. Stay tuned.

@ThienNDDi
Copy link

@fishcharlie Can i use your code in this PR and will DAX work? If it doesn't work, i have to change every thing feature from dynamoose to DAX. Am i right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants