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

[SM-1086] Improve TS bindings #398

Merged
merged 8 commits into from
Feb 8, 2024
Merged

[SM-1086] Improve TS bindings #398

merged 8 commits into from
Feb 8, 2024

Conversation

dani-garcia
Copy link
Member

@dani-garcia dani-garcia commented Dec 4, 2023

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Implement support for projects, accesstokenlogin and response unwrapping. Also added a small example

@bitwarden-bot
Copy link

bitwarden-bot commented Dec 4, 2023

Logo
Checkmarx One – Scan Summary & Details99c45696-0e84-4df1-bbb8-7164bf3568ec

No New Or Fixed Issues Found

@dani-garcia dani-garcia force-pushed the ps/improve-ts-bindings branch from 98e2c07 to 76b3e87 Compare December 13, 2023 12:33
@dani-garcia dani-garcia requested a review from Hinton January 10, 2024 15:49
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@1595306). Click here to learn what that means.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #398   +/-   ##
=======================================
  Coverage        ?   57.76%           
=======================================
  Files           ?      168           
  Lines           ?     9912           
  Branches        ?        0           
=======================================
  Hits            ?     5726           
  Misses          ?     4186           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hinton Hinton requested a review from coltonhurst January 11, 2024 11:28
@Hinton
Copy link
Member

Hinton commented Jan 11, 2024

This should probably go through SM review and QA process before merging. From a quick glance it looks to follow the other language bindings. @coltonhurst do we want to look into deprecating the napi bindings and swap to the wasm for gh actions?

@coltonhurst
Copy link
Member

coltonhurst commented Jan 25, 2024

This should probably go through SM review and QA process before merging. From a quick glance it looks to follow the other language bindings. @coltonhurst do we want to look into deprecating the napi bindings and swap to the wasm for gh actions?

Sorry for the delayed response. Yes, I think that would be a good thing to take care of, I can create an internal ticket for this. The work deprecating the napi bindings and swapping to wasm for gh actions shouldn't block this PR though, correct?

coltonhurst
coltonhurst previously approved these changes Jan 25, 2024
@dani-garcia dani-garcia changed the title Improve TS bindings [SM-1086] Improve TS bindings Jan 29, 2024
Copy link
Member

@coltonhurst coltonhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, only other thing I can think of is to add a readme to the languages/js directory that helps explain how to build and run the example project, kind of like we have in the other language directories. Still approved though!

(The ruby one also has a changelog file, which might be beneficial, but that could also wait until we do a release.)

@dani-garcia dani-garcia merged commit 575dd19 into main Feb 8, 2024
58 of 59 checks passed
@dani-garcia dani-garcia deleted the ps/improve-ts-bindings branch February 8, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants