feat: add `redirect` request option

octokit / request.js

Send parameterized requests to GitHub’s APIs with sensible defaults in browsers and Node

MIT License

228 stars 60 forks source link

feat: add `redirect` request option #636

Closed mitchcapper closed 5 months ago

mitchcapper commented 1 year ago

Resolves #635

Before the change?

Setting {request: {redirect:"value"} } does nothing

After the change?

Setting the redirect property on the request sub property is properly passed to fetch for the request.

Pull request checklist

[ ] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Is it breaking? not really, this is the desired behavior, I can't imagine anyone has a valid use case to set the redirect option and have it ignored.

Please see our docs on breaking changes to help!

[ ] Yes
[X] No

wolfy1339 commented 1 year ago

The option was removed in #599 / #612

We removed all custom options, in favour of users using wrapping fetch with their options

Do we want to reintroduce it though? @gr2m

gr2m commented 1 year ago

It's not a custom option though? I'm not quite sure if we discussed removing the redirect option explicitly, I thought that's one we wanted to keep passing through?

mitchcapper commented 1 year ago

After discussion in #635 I did think this was the desired current behavior so meant to close this PR. I am happy to add a test case to ensure this option works if it is desired for this feature to be re-introduced.

gr2m commented 1 year ago

@wolfy1339 can you recall specifically why we removed the redirect option, given that it's a native fetch option? It might be related to the fact that (I think) it behaves differently in browsers and backend environments due to CORS restrictions that only apply in browsers?

wolfy1339 commented 1 year ago

I don't know.

gr2m commented 1 year ago

It's odd to me that we left signal but removed request.

I think for now I would leave it as is as we have a working workaround. We can revisit the discussion as more folks experience the lack of redirect and other native fetch options as friction

wolfy1339 commented 5 months ago

Requires https://github.com/octokit/types.ts/pull/630

github-actions[bot] commented 5 months ago

:tada: This PR is included in version 9.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: