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

What is the reason to patch the fetch? #25573

Closed
artalar opened this issue Oct 27, 2022 · 19 comments
Closed

What is the reason to patch the fetch? #25573

artalar opened this issue Oct 27, 2022 · 19 comments

Comments

@artalar
Copy link

artalar commented Oct 27, 2022

This looks so unpredictable:

  • works only for get and head (Many APIs request data via post)
  • do not support XMLHttpRequest / WebSocket / WebTransport / postMessage and other side-effects
  • you already have a bad practice with console.log patching (one of the feedback)
  • request.json() will parse and return a new data each time, so dependent effects will recall anyway?
  • finally, it just super bad pattern to patch globals, it always leads to problems eventually. Next.js already holds "next" word in fetch options, which disallow to use it in feature platform updates.

So, what is the reason to patch the fetch?

@ecyrbe
Copy link

ecyrbe commented Oct 27, 2022

I agree, patching fetch is a bad idea.
This should definitely be done in another way.

@judehunter
Copy link

This is a seriously concerning choice

@Urthen
Copy link

Urthen commented Oct 27, 2022

This is an absurd breach of what developers expect out of something like React. Don't monkeypatch existing, standardized browser APIs. If you want to build your own fetch wrapper, fine - but invisibly altering normal browser behavior like this is asking for problems down the road.

@ColeyG
Copy link

ColeyG commented Oct 27, 2022

If you don't think this is malicious (or could become so) you aren't paying attention

@sebmarkbage
Copy link
Collaborator

Wait for the RFC.

@Urthen
Copy link

Urthen commented Oct 27, 2022

Aren't RFCs normally written before code is merged into main?

@sebmarkbage
Copy link
Collaborator

Our RFCs are almost never written before code. We iterate before we're comfortable with it. We don't just publish any first idea that comes to us.

@eps1lon
Copy link
Collaborator

eps1lon commented Oct 27, 2022

This code is not part of a stable release. You won't see this in your app if you're just installing @latest versions.

This is better discussed once the RFC is published.

@ecyrbe
Copy link

ecyrbe commented Oct 27, 2022

@sebmarkbage so maybe keep it in a branch, merging experiments to main is bad if you need to rollback.

@kassens
Copy link
Member

kassens commented Oct 27, 2022

We prefer working on main to avoid conflicts at the cost of needing to iterate on the design there.

Let's wait for the RFC to continue discussion there!

@kassens kassens closed this as completed Oct 27, 2022
@lovetingyuan
Copy link

自以为是的react团队

@IvanAdmaers
Copy link

React & Next might slow web development due to this stupid action. Monkeypatching it’s always bad. Especially, when you do this for native browsers API. It seems like Facebook hired some freshman’s to develop React. It can’t did a developer that has any experience or can think at least one day ahead. React and Next were my favorite technologies but now I can’t say it anymore. Damn, I really can’t believe that we make this mistake again. Didn’t we have enough problems with such monkeypatching lovers as PrototypeJS? Gosh, I just can’t believe that React team made this mega stupid action. God save the XHR…

@silverwind
Copy link

silverwind commented Jan 27, 2023

This is bound to cause issues because it both changes the identify of globalThis.fetch as well as globalThis.fetch.toSource().

I strongly recommend against this monkey-patching and I would probably avoid using any react version that does. It's simply not the business of a DOM rendering to replace the global fetch, at least not without an opt-in mechanism.

@GottZ
Copy link

GottZ commented Feb 2, 2023

I just want to leave this here for you to think about and for anyone coming after me to see that there is an actual caching implementation present these devs here seem to not know about:
https://developer.mozilla.org/en-US/docs/Web/API/Request/cache
in addition to it being a thing, it also stores data in an area that is not garbage collected by v8

you are redefining the wheel here AND pollute global objects.

the existing spec also handles this button correctly:
image
so just you know.. you are breaking dev tools here..

and you can't assume all api's are state-less

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2023

Again, this change is not a part of any stable release. We've heard your feedback, and will adjust the behavior changes to be opt-in (although we likely will encourage patching). We'll post an RFC with the updated proposal.

Edit: since people are doubting this comment, I'll reiterate we will make not do patching by default in React itself (but frameworks would be welcome to make or not make this choice).

@supuwoerc
Copy link

+1,no intrusive changes should be made

@stewones
Copy link

stewones commented Sep 5, 2023

I guess we all appreciate your response @gaearon but it doesn't answer the original question. what's the reasoning behind monkey patching the native fetch? I'm legitimately curious.

@facebook facebook deleted a comment from daukantas Apr 24, 2024
@gaearon
Copy link
Collaborator

gaearon commented Apr 24, 2024

Just to circle back on this — as described earlier, we’re removing the patching ahead of the stable release. #28896

Note it was removed from client builds several days before this issue was filed (#25540) so the only remaining case was related to doing it in the RSC environment (which is where it’s being removed now).

@OZZlE
Copy link

OZZlE commented Oct 28, 2024

Is this part of the latest stable release of React now? And next.js? We seem to still not get the behavior in latest next.js and I cannot find any information about it

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

No branches or pull requests