discordjs / discord.js-modules

Modularisation of discord.js (WIP)
Apache License 2.0
192 stars 30 forks source link

fix(RequestManager): optional headers prioritized over defaults #91

Closed jpbberry closed 2 years ago

jpbberry commented 2 years ago

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

I made this change in order to unlock higher functionality without having to implement every possibility.

With this, it will allow developers to change headers regardless of what is implemented by default in the rest package.

Example

The reason I first stumbled across this issue is when attempting to use the djs rest package for OAuth2 token grants. This endpoint only accepts bodies formatted with urlencoded. And because no matter what, if body isn't null, it will set the Content-Type header to application/json.

The reason for this is seen here

} else if (request.body != null) {
    // Stringify the JSON data
    finalBody = JSON.stringify(request.body);
    // Set the additional headers to specify the content-type
    additionalHeaders = { 'Content-Type': 'application/json' };
}

With this change it would allow the following to produce the expected result:

rest.post('/oauth2/token', {
  body: new URLSearchParams(grant_request_object).toString(),
  headers: {
    'Content-Type': 'application/x-www-form-urlencoded' // this part in particular
  }
})

This also frankly makes the case for a parser function, that would allow the user to change what is truly passed to the fetch (rather than being forced to use JSON.stringify), because it is only luck that this encoding can still be passed through JSON.stringify and just return the same string, something like FormData, even though there is an attachments option, would just break. However, that's outside of the scope of this PR.

Status and versioning classification:

This could and could not be a breaking change. It could create issues with existing code, though I think the current functionality could be considered unexpected behavior, as it's completely removing things from the options in the first place.

jpbberry commented 2 years ago

This would fix #90 btw. But doesn't address the larger issue that I touched on in the parser dialogue.