Skip to content

feat: proactive connect #818

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

Merged
merged 1 commit into from
Apr 25, 2023
Merged

feat: proactive connect #818

merged 1 commit into from
Apr 25, 2023

Conversation

manchuck
Copy link
Contributor

@manchuck manchuck commented Apr 24, 2023

Adding in proactive connect

Description

Motivation and Context

Testing Details

Example Output or Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2023

Codecov Report

Merging #818 (82ad1a3) into 3.x (82399a4) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              3.x     #818      +/-   ##
==========================================
+ Coverage   78.94%   79.01%   +0.06%     
==========================================
  Files         104       89      -15     
  Lines        1211     1215       +4     
  Branches      234      239       +5     
==========================================
+ Hits          956      960       +4     
  Misses        255      255              
Impacted Files Coverage Δ
packages/server-client/lib/transformers.ts 96.00% <ø> (ø)
packages/proactive-connect/lib/proactiveConnect.ts 100.00% <100.00%> (ø)
packages/server-client/lib/client.ts 98.41% <100.00%> (+0.02%) ⬆️
packages/server-sdk/lib/vonage.ts 93.75% <100.00%> (ø)

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@manchuck manchuck force-pushed the proactive-connect branch from 423546d to 1c3f294 Compare April 24, 2023 20:17
@manchuck manchuck force-pushed the proactive-connect branch from 1c3f294 to 82ad1a3 Compare April 24, 2023 20:21
@manchuck manchuck marked this pull request as ready for review April 24, 2023 20:22
@manchuck manchuck requested a review from dragonmantank April 24, 2023 20:22
Copy link
Contributor

@dragonmantank dragonmantank left a comment

Choose a reason for hiding this comment

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

Just a couple of questions, otherwise looks good

file: string,
): Promise<ImportFileResponse> {
const itemsForm = new FormData();
itemsForm.setBoundary(this.FORM_BOUNDARY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there not a library that can handle creating multipart form bodies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lib does, but it creates a different form on each instance. We set the form boundary here for testing purposes. I went through several libs to parse multi-parts, and they were all crap except for the one we are using. However that one needs the boundary explicitly set.

@manchuck manchuck merged commit a00efc4 into 3.x Apr 25, 2023
@manchuck manchuck deleted the proactive-connect branch April 25, 2023 20:07
manchuck added a commit that referenced this pull request Apr 28, 2023
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.

3 participants