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

refactor(monitoring): replace request with helix-fetch #95

Merged
merged 6 commits into from Jul 2, 2020

Conversation

rofe
Copy link
Collaborator

@rofe rofe commented Jul 1, 2020

Fix #93

@rofe
Copy link
Collaborator Author

rofe commented Jul 1, 2020

Statuspage tests with nock are still failing in spite of forcing http1: Response JSON reads:

{
  "error": "Could not authenticate"
}

@kptdobe
Copy link

kptdobe commented Jul 1, 2020

I have the same issue with POST requests.

@github-actions
Copy link

github-actions bot commented Jul 1, 2020

This PR will trigger no release when merged.

@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #95 into master will decrease coverage by 1.56%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   94.47%   92.91%   -1.57%     
==========================================
  Files           9        9              
  Lines         326      381      +55     
==========================================
+ Hits          308      354      +46     
- Misses         18       27       +9     
Impacted Files Coverage Δ
src/statuspage/cli.js 96.59% <81.25%> (+0.48%) ⬆️
src/newrelic/alerts.js 87.50% <85.41%> (-2.98%) ⬇️
src/newrelic/synthetics.js 91.66% <90.00%> (-1.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 845c9a4...380e25b. Read the comment docs.

@rofe
Copy link
Collaborator Author

rofe commented Jul 1, 2020

I have the same issue with POST requests.

@kptdobe, @stefan-guggisberg fixed it (thanks!) by moving the process.env.HELIX_FETCH_FORCE_HTTP1 definition to the top of the file.

@rofe rofe marked this pull request as ready for review July 1, 2020 15:42
@rofe rofe requested a review from kptdobe July 1, 2020 15:43
channel_ids: channelId,
policy_id: policy.id,
},
body: `channel_ids=${encodeURIComponent(channelId)}&policy_id=${encodeURIComponent(policy.id)}`,
Copy link
Collaborator Author

@rofe rofe Jul 1, 2020

Choose a reason for hiding this comment

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

If anyone knows of a more elegant way to submit url encoded form data, let me know 😄

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, URLSearchParams. You also need to set the content-type: application/x-www-form-urlencodedheader.

The Fetch API allows both URLSearchParams and FormData as body: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#Body

helix-fetch doesn't support it yet. Feel free to create a feature request issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rofe rofe changed the title refactor(statuspage): replace request with helix-fetch refactor(monitoring): replace request with helix-fetch Jul 1, 2020
Copy link
Contributor

@stefan-guggisberg stefan-guggisberg left a comment

Choose a reason for hiding this comment

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

See my comment. Apart from that LGTM.

@rofe
Copy link
Collaborator Author

rofe commented Jul 1, 2020

Using URLSearchParams

@rofe
Copy link
Collaborator Author

rofe commented Jul 1, 2020

Created adobe/fetch#67

@rofe rofe merged commit 8145bf7 into master Jul 2, 2020
@rofe rofe deleted the replace-request branch July 2, 2020 12:57
@adobe-bot
Copy link

🎉 This PR is included in version 1.11.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

request is deprecated; replace with helix-fetch
5 participants