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(proxy): transparently forward errors when passing ofetch #466

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

Aareksio
Copy link
Contributor

@Aareksio Aareksio commented Jul 27, 2023

πŸ”— Linked issue

#464

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

As discussed in #464 and nitrojs/nitro#1489.

ProxyOptions type updated to allow overriding the default value (true).


Is it breaking change?

I would say so, users may depend on error masking. This is not strictly a bug. Mandatory xkcd: https://xkcd.com/1172/


I personally dislike this solution a lot; if we allow freedom of passing fetch implementation, it should be up to the callee to configure the behavior of their implementation exactly as they like. If it's required to pass option to do so, they already can do so with fetchOptions.

Forcing ignoreResponseError: true is implicitly re-configuring non-default user-passed implementation. I think better approach is to allow the freedom along with passing the responsibility. Another edge-case is when user passes fetch implementation which also supports ignoreResponseError or throws on unknown parameters - both sound plausible to me.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@pi0 pi0 changed the title fix(proxy)!: make ofetch.$raw forward errors transparently fix(proxy): transparently forward errors when passing ofetch Jul 27, 2023
@pi0
Copy link
Member

pi0 commented Jul 27, 2023

I personally dislike this solution a lot; if we allow freedom of passing fetch implementation, it should be up to the callee to configure the behavior of their implementation exactly as they like. If it's required to pass option to do so, they already can do so with fetchOptions.

I think both implementation can be changed and also this option can be directly still overridden by users.

I consider it as a good fix because the h3 intended behavior was transparently bypassing. It was mainly my bad that forgot about it when integrating with nitr. (reason of using ofetch was direct calls not hiding them)

@Aareksio
Copy link
Contributor Author

Either solution solves the problem. If you like it, that's good enough! Much better title too! ❀️

I am still a little worried about this one guy who uses error masking to hide secrets from consumers, but if you consider it non breaking, be it πŸ˜‚

@pi0 pi0 merged commit 9bd85a3 into unjs:main Jul 27, 2023
4 checks passed
@pi0
Copy link
Member

pi0 commented Jul 27, 2023

You can try it on nightly channel now (check little new section in readme!)

@pi0 pi0 mentioned this pull request Aug 4, 2023
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.

2 participants