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

fix(Rest): resolved a regression, added retried AbortError #4852

Merged
merged 2 commits into from
Sep 29, 2020

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented Sep 26, 2020

Please describe the changes this PR makes and why it should be merged:

#4835 introduced a regression in 4xx errors, the following chunks of code are not identical - the old one rejected with DiscordAPIError if there was a 4xx error, but the new one throws, and then re-throws but as a HTTPError:

try {
const data = await parseResponse(res);
if (res.status >= 400 && res.status < 500) {
return reject(new DiscordAPIError(request.path, data, request.method, res.status));
}
return null;
} catch (err) {
return reject(new HTTPError(err.message, err.constructor.name, err.status, request.method, request.path));
}

try {
const data = await parseResponse(res);
if (res.status >= 400 && res.status < 500) {
throw new DiscordAPIError(request.path, data, request.method, res.status);
}
return null;
} catch (err) {
throw new HTTPError(err.message, err.constructor.name, err.status, request.method, request.path);
}

My apologies for anyone affected, even though nobody reported this issue (yet) since the names and properties are nearly identical for error handlers that don't check for instanceof. But the good news, retried AbortErrors!

Stack Traces and Types

Running the following code, and using @klasa/type to retrieve the output's type in eval:

message.channel.send('a'.repeat(2001));

Before this patch:

DiscordAPIError: Invalid Form Body
content: Must be 2000 or fewer in length.
    at RequestHandler.execute (.../node_modules/discord.js/src/rest/RequestHandler.js:157:13)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async RequestHandler.push (.../node_modules/discord.js/src/rest/RequestHandler.js:39:14)
    at async Object.eval (.../dist/commands/System/Admin/eval.js:74:26)
    at async Object.run (.../dist/commands/System/Admin/eval.js:19:49)
    at async default_1.runCommand (.../dist/monitors/commandHandler.js:58:38)
    ...

Type:

Promise<HTTPError>

The type is incorrect, the error thrown must be an instance of DiscordAPIError if it's related to an error thrown from the API.

After this patch:

DiscordAPIError: Invalid Form Body
content: Must be 2000 or fewer in length.
    at RequestHandler.execute (.../node_modules/discord.js/src/rest/RequestHandler.js:154:13)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async RequestHandler.push (.../node_modules/discord.js/src/rest/RequestHandler.js:39:14)
    at async Object.eval (.../dist/commands/System/Admin/eval.js:74:26)
    at async Object.run (.../dist/commands/System/Admin/eval.js:19:49)
    at async default_1.runCommand (.../dist/monitors/commandHandler.js:58:38)
    ...

Type:

Promise<DiscordAPIError>

With this patch, the error's type is the correct one.


Fixes #3471

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@neofutur
Copy link

please push this, we badly need a fix for #3471

@kyranet
Copy link
Member Author

kyranet commented Sep 29, 2020

There's no rush, giving time before merging helps getting more eyes on it, which can lead to more people trying it out. The more, the better, as they can find issues we didn't.

This PR will be included in the 12.4.0 release, that's for sure :)

@iCrawl iCrawl merged commit d234165 into discordjs:master Sep 29, 2020
@kyranet kyranet deleted the fix/retried-abort-errors branch September 29, 2020 16:36
GiorgioBrux pushed a commit to GiorgioBrux/discord.js that referenced this pull request Oct 1, 2020
GiorgioBrux pushed a commit to GiorgioBrux/discord.js that referenced this pull request Oct 17, 2020
@flav-code
Copy link

image
image
i have this problem

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.

Retry limit on HTTPError [AbortError]
7 participants