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

Mirror superagent behaviour in call back for creating errors #14

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

repl-mike-roest
Copy link
Contributor

@repl-mike-roest repl-mike-roest commented Oct 19, 2023

In our use of this delay module we noticed a difference in our end callback behaviour when just including the retry-delay. This is porting https://github.com/ladjs/superagent/blob/master/src/node/index.js#L893-L908 this section of the core superagent callback into the retry-delay callback to create errors on a non-ok response. to mirror the core superagent behaviour

@repl-mike-roest
Copy link
Contributor Author

@luispabon any chance to get this merged?

Thanks

@luispabon
Copy link
Owner

Thank you Mike. This significantly changes the error handling interface, I'd need to issue a new major version to ensure semver is respected. Could you please let me know which version of superagent you at?

src/index.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@repl-mike-roest
Copy link
Contributor Author

Thank you Mike. This significantly changes the error handling interface, I'd need to issue a new major version to ensure semver is respected. Could you please let me know which version of superagent you at?

We were on 3.6.0 and I also tried it on 8.1.2 they both appear to have the same error synthesizing behaviour on nonOk responses.

@repl-mike-roest
Copy link
Contributor Author

repl-mike-roest commented Oct 23, 2023

@luispabon yep no problem added tests for the new error synthesizing cases. And yah I think a new major version makes sense. Although it's bringing the behaviour inline with the core superagent it is a major change from this plugin's point of view.

@repl-mike-roest
Copy link
Contributor Author

@luispabon sorry to be a pain, just wondering if you have a rough idea of when you would have time to get this in?

Thanks

@luispabon luispabon merged commit 35f82c5 into luispabon:master Nov 2, 2023
6 checks passed
@luispabon luispabon mentioned this pull request Nov 2, 2023
@luispabon
Copy link
Owner

@repl-mike-roest
Copy link
Contributor Author

https://github.com/luispabon/superagent-retry-delay/actions/runs/6733950566 <-- published 3.0.0

You rock thanks so much!

@repl-mike-roest repl-mike-roest deleted the fix-errors-on-callback branch November 2, 2023 16:17
@luispabon
Copy link
Owner

No, thanks for your contribution 👍🏽

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.17%. Comparing base (9b3dd9e) to head (01926c9).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   84.90%   91.17%   +6.27%     
==========================================
  Files           1        1              
  Lines          53       68      +15     
==========================================
+ Hits           45       62      +17     
+ Misses          8        6       -2     

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

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