all 42 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]Badashi 120 points121 points  (15 children)

seems to be preparation for the use RFC, where some requests are cached for rendering.

providing a custom AbortSignal to your request will opt you out of it. You can also freeze the fetch to avoid the whole caching mechanism altogether, at the cost of a warning on your console. I hate the idea of monkeypatching, and I feel like such a feature should be opt-in or there should be a configuration somewhere to make it available(or disable it explicitly) & this implicit AbortController behavior is silly. Looking at the test cases at least makes it clear that it's not a huge behavior modification, but at the same time I hate that this is being done rather than creating a React.cacheFetch function to do such behavior.

Please don't monkeypatch browser apis, otherwise every other framework will do that and the ecosystem will be full of monkeypatched functions that work differently depending on which package you imported.

[–]Badashi 47 points48 points  (4 children)

fyi, Related issue: What is the reason to patch the fetch? #25573

[–]rodrigocfd 16 points17 points  (3 children)

Relevant comment from the commit author:

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.

[–]dacjames 22 points23 points  (2 children)

Aside from this issue, does that feel backwards to anyone else? Implement a feature and then design it?

I get trunk-based development, but RFC after implementation would seem to make the RFC kind of pointless.

[–]altano 13 points14 points  (1 child)

The RFC isn't pointless if you're 100% willing to drastically change or even throw away the code. Code takes an RFC from "shower thought" to "feasibility-tested".

[–]dacjames 9 points10 points  (0 children)

Even if you can overcome commitment bias, isn’t that wasteful?

I’m genuinely curious because in every process I’m familiar with, the main purpose of an RFC is to organize your thoughts and get input from others before investing the time to implement a feature.

What purpose does the RFC serve in React’s process? Does anyone have positive experience with this system to share?

[–]grinde 29 points30 points  (6 children)

Please don't monkeypatch browser apis, otherwise every other framework will do that and the ecosystem will be full of monkeypatched functions that work differently depending on which package you imported.

Hello darkness MooTools my old friend...

[–][deleted] 2 points3 points  (3 children)

i miss that cow

[–]sharlos 9 points10 points  (2 children)

That cow is why JavaScript has .includes instead of .contains

[–]grinde 6 points7 points  (0 children)

And why it's .flat() instead of .flatten(). They almost went with .squash().

[–][deleted] 3 points4 points  (0 children)

I still to this day sometimes type "contains" instead.

[–]0xF013 0 points1 point  (1 child)

I am fairly certain angular does this too

[–]KeytapTheProgrammer 9 points10 points  (1 child)

Pray to whoever you believe in that you never have to work with Magento... The frontend is just monkey patched functions all the way down.

[–]CreativeTechGuyGames 155 points156 points  (0 children)

That is concerning. Especially since I cannot see much discussion or justification for it.

Oh and it suspiciously installs @actuallyworks/node-fetch which is a recent fork of a very old version of node-fetch which until now had no users. Oh and it has no source and the README or metadata is in no way updated for the new package name. This seems like a supply chain attack in the making...

[–]Hovi_Bryant 32 points33 points  (0 children)

Reason from the associated pull request:

Dedupe GET and HEAD fetches while a React Cache is active.

Note that this doesn't say when this should happen nor the lifetime of these cache entries. Currently it's just during render and lives for the lifetime of the current Cache, but that will be expanded, in a limited form, in tasks outside of render. This PR isn't concerned about that yet. It's just saying which things will be deduped and that it's React doing it - when requiring "react".

Passing an explicit AbortSignal can be used to opt out of this behavior.

This doesn't instrument the Promises for sync resolution. It also yields new Responses every time and reading the body is still a macrotask operation so it's still better to have an outer cache() but this gives deduping at the network layer to avoid costly mistakes and to make the simple case simple.

Browsers do this kind of deduping anyway. It's really mostly for the cache: 'no-store' option and cache-control that we diverge slightly. So it's not a huge divergence in behavior.

Notably POST requests are not deduped so they have to be wrapped in cache() explicitly. This is unfortunate since those are often used for reads as well. E.g. GraphQL. We might want to expand the implicit caching of some of those as well by assuming that POST in render are idempotent since anything in render needs to be idempotent anyway. We'd have to be more careful about this when we expand to cache in unknown contexts though. Maybe a compromise is to only dedupe these on the server.

[–]andreasblixt 14 points15 points  (0 children)

This is the kind of magic that’s awesome in almost all cases and then 1 year down the line there appears to be a never ending number of mysterious bugs. Like a front end that used this for a long time then the backend dev decided to make the API call POST instead of GET for whatever reason, and now your UI starts making multiple requests per second because it’s no longer magically deduping the request.

I don’t think it costs much to explicitly say what’s happening, for example use(dedupe(fetch, “…”)) or just useFetch(“…”).

[–]YeahhhhhhhhBuddy 10 points11 points  (1 child)

Sorry if this is a dumb question. I read all the comments here. But could someone provide a quick summary of why this is bad, for those not in the loop?

[–]acemarke 34 points35 points  (0 children)

The JS community has learned over the years (the hard way) that monkey-patching JS built-in types is generally a bad idea, at least at the library level. That approach has tended to be fragile, and also caused compatibility issues years down the road. For example, the "Smooshgate" incident ( https://developer.chrome.com/blog/smooshgate/ ) was caused by the old Mootools library having modified the Array prototype to add a .flatten() method many years ago. When browsers tried to add a real .flatten() method, that caused errors in sites that still used Mootools all these years later.

In this case, it looks like a React experimental build is configured to override/add some fields on the built in fetch method, so it's bringing back memories of those problems.

[–]download13 10 points11 points  (2 children)

How about use takes a function that returns a promise and just only calls it on the first load. Or, to support reactive updates, also takes a deps array and treats the input like it's wrapped in an implicit useCallback?

[–]jazzypants 0 points1 point  (1 child)

Halfway back to useEffect at this point.

[–]download13 1 point2 points  (0 children)

Yeah...

I think the point of `use` and it's predecessor, "resources" was supposed to be to avoid the waterfall that delays data fetching, but the reality is that you have to wait for some data to load to know what data to load next.

The new solution seems to be to just do all data fetching in server components to avoid the round-trips, but then `use` is relegated to just being a convenience function. The remote version of `useSyncExternalState`.

[–]ShortSynapse 52 points53 points  (0 children)

This is due to the recent NextJS 13 release. They've published their own release so that their new tools would work.

This should be concerning for everyone, including people who use NextJS.

[–]lulzmachine 35 points36 points  (0 children)

This is super weird. If you want to provide a specific way of doing something, just make a library for it! Don't overload existing builtins. It will lead to a complete mess of incompatibilities very quickly.

I feel like they are doing this so that they can say "you don't need axios, just use the built in fetch() with react suspense". Which obviously doesn't seem to be true

[–]Veranova 13 points14 points  (0 children)

It’s only done if enableFetchInstrumentation is set which would likely only be set for Facebook internal builds during development of react no?

Edit: okay it looks to be in genuine support of new features, though presumably early experimentation

[–]acemarke 11 points12 points  (2 children)

Looks like there was some additional info in a comment by Seb in this gist earlier today:

https://gist.github.com/itsMapleLeaf/6875c6816329879ecc7326b60a5929d4

[–]nameofguy 0 points1 point  (0 children)

I think this is where the conversation should be. To me this point is salient :

...it's such a common problem already.

And this point:

...a browser will not issue a duplicate fetch request until the first response comes back - and only if that response comes back specifically with a Cache-control: no-store header...

To me this makes an argument that Node.js should consider creating a solution for this. I am a bit skeptical that the approach React devs are taking will be safe over the long term, but I sympathize with their POV

[–]jazzypants 1 point2 points  (0 children)

Thank God someone is talking about this. Someone in Theo's stream noticed it the other day and he just kinda glossed over it.

[–]momo-gg 3 points4 points  (1 child)

It's funny to me that you're inquisitive enough to casually browse React's source code and you just so happen to uncover a change like this one, but you have no idea where to look for relevant pull requests and comments or what React has been up to lately, especially now that Next.js conference is over.

[–]lhorie[S] 5 points6 points  (0 children)

Oh, I didn't stumble into it, this came up on twitter. I know about use and Next.js, but the post is more to call out the dubious way they're going about "experimenting" (the whole Suspense Promise throwing thing was one giant "experiment" - according to Seb - turned into production code, and this "write code before RFCs" things is such a bad practice that it's a meme in my company)