dapr / js-sdk

Dapr SDK for Javascript
Apache License 2.0
198 stars 84 forks source link

feat(state): add optional attributes in state management APIs #476

Closed SoTrx closed 1 year ago

SoTrx commented 1 year ago

Description

This PR adds support for the concurrency and consistency attributes, and adds etag headers where they were missing. However, there is still one thing that remains to be done. The documentation example about Etags specifies that get request should return the etag value. The get request currently returns a plain string and the etag is even ignored in the HTTP client execute method.

I didn't make any changes there, as it would be quite a big breaking change. How should we go forward with this ?

Issue reference

Please reference the issue this PR will close: #475

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

codecov[bot] commented 1 year ago

Codecov Report

Merging #476 (0b1d425) into main (d742c36) will increase coverage by 0.63%. The diff coverage is 27.05%.

@@            Coverage Diff             @@
##             main     #476      +/-   ##
==========================================
+ Coverage   38.17%   38.81%   +0.63%     
==========================================
  Files          80       82       +2     
  Lines        8105     8183      +78     
  Branches      346      370      +24     
==========================================
+ Hits         3094     3176      +82     
- Misses       4947     4948       +1     
+ Partials       64       59       -5     
Impacted Files Coverage Δ
src/implementation/Client/GRPCClient/state.ts 5.51% <0.00%> (-0.80%) :arrow_down:
src/implementation/Client/HTTPClient/state.ts 11.76% <3.33%> (-9.67%) :arrow_down:
src/utils/Client.util.ts 82.14% <56.00%> (+36.38%) :arrow_up:
src/enum/StateConcurrency.enum.ts 100.00% <100.00%> (ø)
src/enum/StateConsistency.enum.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

XavierGeerinck commented 1 year ago

Thank you for your PR! I reviewed it and added some remarks, could you take a look at those? Also please sign the DCO so we can approve :)

XavierGeerinck commented 1 year ago

@SoTrx could you fix the last few multi lines and newline comments? Feel free to resolve my suggestions and I will re-review ASAP .

P.S. before committing please run npm run pretty-fix as the tests are failing due to missing code stylings

SoTrx commented 1 year ago

Sorry, I had quite a hard time with the DCO compliance with my current setup (I'll squash any remaining inconsistencies), I even forgot to run prettier. This should be fixed !

XavierGeerinck commented 1 year ago

Looks amazing! Thank you!! Will run the tests and await the results then we can merge it in :)

XavierGeerinck commented 1 year ago

image

could you fix those? @SoTrx

SoTrx commented 1 year ago

Sure, plain strings shouldn't be used here, I'll patch up the test

SoTrx commented 1 year ago

So enums are now used correctly. Since some calls require the use of enum string values, I've rewritten some logic. In doing so, I've had to create a "createHTTPQueryParam" method that's a bit more general than the existing "createHTTPMetadataQueryParam".

However I did not delete createHTTPMetadataQueryParam, as this would also impact pubsub, which is out of scope for this PR.

If you are happy with the new implementation of the method, you can remove the old method, as both should be equivalent.

XavierGeerinck commented 1 year ago

Could you fix the below?

/home/runner/work/js-sdk/js-sdk/src/utils/Client.util.ts
Error:   34:10  error  'IStateOptions' is defined but never used. Allowed unused vars must match /^_/u  @typescript-eslint/no-unused-vars
XavierGeerinck commented 1 year ago

Thanks! Tests are passing, so merging it now :) thank you for your amazing contribution!!