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.

Sign up for GitHub

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

Jump to bottom

feat: Add error event and improve error handling #185

Merged
merged 3 commits into from
Feb 20, 2019
Merged

feat: Add error event and improve error handling #185

merged 3 commits into from
Feb 20, 2019

Conversation

offirgolan
Copy link
Collaborator

@offirgolan offirgolan commented Feb 19, 2019 •
edited
Loading

Resolves #181

server.any().on('error', (req, error) => {
  console.error(error);
  process.exit(1);
});

@offirgolan offirgolan added the enhancement ✨ New feature or request label Feb 19, 2019
offirgolan
offirgolan commented Feb 19, 2019
View reviewed changes
packages/@pollyjs/adapter/src/index.js
error || new Error('[Polly] Request failed due to an unknown error.');

await pollyRequest._emit('error', error);
await this.respondToRequest(pollyRequest, error);
Copy link
Collaborator Author

@offirgolan offirgolan Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Call respondToRequest with an error if the request failed so that the adapters can gracefully handle it.

offirgolan
offirgolan commented Feb 19, 2019
View reviewed changes
packages/@pollyjs/core/src/-private/logger.js
@@ -13,10 +13,6 @@ export default class Logger {
this.groupName = null;
}

get enabled() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing dead/unused code.

jasonmit
jasonmit approved these changes Feb 19, 2019
View reviewed changes
packages/@pollyjs/adapter-xhr/tests/utils/xhr-request.js
@@ -22,7 +22,7 @@ export default function request(url, obj = {}) {
xhr.status === 204 && xhr.responseText === '' ? null : xhr.responseText;

return new Response(responseBody, {
status: xhr.status,
status: xhr.status || 500,
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is just pertaining to a test util but can the fallback mask a future bug in our test suite?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will only happen when status is 0 which occurs when we do xhr.error(). Do you have a better way around this? Response wont accept a status code of 0

packages/@pollyjs/adapter/src/utils/stringify-request.js

// Remove all adapter & persister config options as they can cause a circular
// structure to the final JSON
['adapter', 'adapterOptions', 'persister', 'persisterOptions'].forEach(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a fix for separate bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a fix for a separate bug that I found when tackling this.

@jasonmit jasonmit merged commit 3694ebc into master Feb 20, 2019
@feinoujc feinoujc mentioned this pull request Feb 21, 2019
Merged
9 tasks
@offirgolan offirgolan deleted the error-event branch July 17, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmit jasonmit jasonmit approved these changes

Assignees
No one assigned
Labels
enhancement ✨ New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@offirgolan @jasonmit