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

feat: add redirect request option #636

Merged

Conversation

mitchcapper
Copy link
Contributor

@mitchcapper mitchcapper commented Sep 23, 2023

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
  • No

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented label Sep 23, 2023
gr2m
gr2m previously requested changes Sep 23, 2023
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Could you add a test please, to avoid a future regression?

@wolfy1339
Copy link
Member

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
Copy link
Contributor

gr2m commented Sep 23, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

gr2m commented Sep 24, 2023

@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
Copy link
Member

I don't know.

@gr2m gr2m changed the title Fixed regression of redirect option being passed through to fetch feat: add redirect request option Sep 24, 2023
@gr2m
Copy link
Contributor

gr2m commented Sep 24, 2023

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
Copy link
Member

Requires octokit/types.ts#630

@wolfy1339 wolfy1339 merged commit 23200c0 into octokit:main Apr 9, 2024
6 checks passed
wolfy1339 pushed a commit that referenced this pull request Apr 9, 2024
* Fixed regression of redirect option being passed through to fetch introduced in #599, and moves it instead to the `requestOption.request` object

* build: upgrade jest to fix test regression with `globalThis` being read-only
Copy link

github-actions bot commented Apr 9, 2024

🎉 This PR is included in version 9.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Redirect option is not passed through...
3 participants