Please Stop Patching Globals
ฝัง
- เผยแพร่เมื่อ 25 ก.ย. 2024
- The drama around overriding global functions in JS is exploding. Artem wrote a great blog post and I took the time to break it down.
SOURCES
kettanaito.com...
/ 1787869125924270547
/ 1788327745925390571
gist.github.co...
/ 1788227053210181702
/ 1787869125924270547
Check out my Twitch, Twitter, Discord more at t3.gg
S/O Ph4se0n3 for the awesome edit 🙏
Thank you for covering this topic, Theo!
If different runtimes start adding these "patches" all over the place we'll end up with the same annoying thing as with SQL - No real standard, 80+ different "flavors", all of which have custom functions and syntax... Ugh...
I wish I could like this 100 times! I am so tired of waisting time to search how to write something in any DB's flavor of SQL. I know what i want to do and I keep wasting time to find how to write it.
The fact that the language even permits such behavior is very concerning.
@@trombecher "window" is an object like any other. You could have it permanently frozen but then you'd break millions of websites that use it for setting global variables.
@@ecodes8498AI is a real help for this lately, still not ideal, hallucinating and stuff... But man, I hope future is bright for this usecase of it, I like that I can just describe what I am doing and it in most cases gives me correct enough answer so I don't have to spend time reading pages of docu or googling
@@trombecher To be honest, this is quite a useful feature. You may want to send some additional header automatically with every fetch request so you patch the fetch method locally in your project, but when people start doing it inside libraries without giving you any choice - That's when that flexibility becomes an obstacle...
I'm really disappointed because even after an hour-long video I still don't have the slightest idea why anyone thinks it's a good idea to patch globals. I really don't see the point - if you want to have custom behavior, just wrap the thing in a function. To use it, you have to import it... is that really so hard?! I'm serious, is that the argument?? Not having to do the import statement?
IKR, absolute madness. Absolutely unacceptable justifications for patching globals, just export that shit.
It's because some libs use default fetch under the hood, so next cannot dedupe it easly
But in my optionion we shoud not be like: "Okey so i just override fetch to fix deduping"
Bug we shoud say: "Ey! Sptil your library into modules, so we can use it with own fetch and give default begavior in some sort of dependency injection, adapter or strategy pattern"
@@untlsn Oh let me, the one putting it together patch global fetch. Heck, make a function that is patchGlobalFetch if you really need to. Just don't do stuff like this by default!
It depends, a lot of times we patch "window" adding props to it
Embrace. Extend. Extinguish. Many frameworks and runtimes are funded by VC money. Although, they will most likely fail because they aren't IBM or Microsoft.
"patching things you don't own" ... that is exactly why this is happening. These frameworks are claiming ownership over these things. If you use next, now you must use the next ecosystem for everything because the patched global forces you to use only next things.
Adopting this pattern _normalizes_ this behaviour and will lead to more and more libraries participating in this, leading tothe normalization of more vendor lock-in.
also like why would you not do `import { fetch } from "@next/fetch";`
@@joshuathomasbirdI really don't understand why they haven't done this. The did a similar thing with their Link component and no one has had issues with it because it's optional.
@@joshuathomasbird yeah i feel like that's a way better solution, then others can also spot that you're using "something similar but seperate" by the import
Guys did you watch his video?
He is explaining it in detail.
The problems come if you import a library that uses fetch under the hood. This library will not import something from next and is because of this inefficient.
Pls watch the video before commenting here.
@@JakobRossner-qj1wo They probably did watch the video, because the reason to "fix other libraries inefficiencies" by changing GLOBAL code under the hood is a shitty solution no matter what. A better solution would be to incentivize libraries to adopt a "fetch-injection" API, allowing clients to change fetch at runtime, this would be a WAY better solution.
"Should we do things the way the standards expect, or the way X product expects" it's unbelievable that this is even a reasonable question to ask.
^ this! The argument Theo seems to bring is that lot of places don’t follow the standard. Yeah, so maybe start following the standard yourself so there’s one less place not following the standard?
@@domints if a standard exists but nobody follows it, is it really a standard?
@@connorskudlarek8598 is it "nobody follows it" or maybe rather "some JS devs decided to ignore it for some dumb reason"?
@@domints I'm just saying, it seems like it isn't "some" devs. That implies a small group of them, or maybe a single organization or something.
It seems that most devs are not following the standards. Which by definition, makes them not standard.
In mechanical engineering, if there is an ASME/ANSI standard, then that's the standard to meet ASME/ANSI. You won't make it past the door if ASME/ANSI is required.
Nothing in software seems to do this though. It's the Wild West. Everything is allowed, standards are suggestions.
@@connorskudlarek8598 you know there’s a lot of software beyond some JS libs? You know, like web browsers? Frameworks for other languages which actually follow standards? Also, even Theo pointed out that out of 3 libs two followed the standard and 1 did not.
Your argument for patching is to improve bad libraries, but fixing them would be much better, de-dupping can break libraries which expect that two calls are made.
Library authors will face confusion because their library broke when an unsuspecting user imported react or used a different runtime that made the behaviour of global functions different.
I'm primarily a back end developer but patching stuff just to get caching/de-duping functionality seems like a bad idea. If you need to 'optimize' requests in your own code then create your own layer for such a thing. Make your own 'fetch' library which has de-duping and caching (and potentially some other stuff) . If the 3rd party library is not optimized then (IMHO) it would be a bad idea for you to go around fixing it by patching low-level primitives that the library uses. Don't use that library or fork it and change its behaviour.
The average React server components dev should not be in charge in creating their own abstraction upon React, just to dedupe requests. Also this will not apply to requests, that React may be doing internally. I agree that it is never an option to patch globals, but to import patched functionality from e.g. React that has the behavior built-in. Then your code and React code can share the same code, the same deduping.
re: Harm #1; automated deduping on a global level is always scary to me as it comes with uncertainty as to what it considers a duplicate request. What happens when I intentionally need to send two very similar requests in close proximity, say a difference in one custom header value? What happens when a library I use is including a trace ID in a header, but I still want _those_ requests deduplicated? Do companies need to start making framework-specific versions of their audit libraries to work around these kinds of patches instead of just providing bindings to shared code? There's no one size fits all and it's a little scary to be injecting those kinds of behaviors into code you don't own.
I disagree with your dedup per request cycle argument/example. It's _incredibly confusing_ if an explicit fetch doesn't actually do the fetch, because somewhere else the same fetch was called. That's a debugging nightmare and frankly, the dedup mechanism is a solution to the wrong problem. You can solve this in a straight forward, explicit and easy to understand manner by wrapping the call via a cache, or you could structure your code so functions that require auth have to be initialized a priori (where the fetch to auth would happen once). In any case, monkey patching standard behavior so you can avoid structuring your code is not convenience, it's madness.
None of this would've been a problem in the first place, if they made their "patched" globals on a separate module
Yeah, then they could also have made more powerful tools. I guess the default should be to also dedupe requests if they come in at the same, before one of them had their result cached, I don't think their fetch does that
I love the fact that in 90% of videos of "article/blog reading", you have the author in chat to give nuances and his oun true meanings to accomplish a better understanding, conclusions and better viewer experience 😊
Overwriting global functionality is disguting and will only lead to chaos. Just make a new function at that point.
Yes you should get 12 15 14. It should be up to you or the library author to explicitly prevent that. If the framework wants to help you they should make an API so it's explicit that you're opting in to behavior that fetch doesn't normally have. Next should have nextFetch because that's what they're doing. If you want to override fetch yourself you can do that. But it should be explicit.
We had a bug a few years ago, I don't remember the details, but we imported fetch from some package which called it just fetch, and it had some weird behavior. To solve the bug, all you had to do is to remove the import statement, but you had to know that this was a custom fetch. There are like 20 imports at the top of the file. That took a few days to find (and it was unclear what exactly caused the problem).
And when the app router came out, the very second I saw vercel's next: { revalidate: 10 } was the moment I sighed deeply. Just freaking call it something else!!! The same with Image component, there's already Image in scope!
Also the new router they also called useRouter which is however imported from a different package. WHY??? The API is different.
Composition is almost always the way to go.
Clojure completely solves this by having namespaced keywords. It's the most simple but perhaps one of the most powerful features in the languages regarding longevity and maintenance.
RE: Content Length
IMO despite what the spec says if you specify you want a header to have a specific value I think it's wrong that it gets overwritten silently.
That sort of thing wastes a lot of dev time, if you override or ignore developer specified values it should at least generate a warning.
The thing with Content-Length is that when you put that value wrong you won’t know what server will do - it’s technically wrong HTTP request, server should reject it. But will it? It might wait for extra data, possibly for long time. Or it’ll see HTTP2 and assume more body data is new request but it’ll se garbage, leading to god knows what. You shouldn’t specify content-length yourself, period. It should get overwritten by library to make sure the content length is what body is.
Headers are a bit of a specific case though. There's also the question of "Is Header-Length a source of truth (!!) or a helping description of body that is the source of truth"?
Here, it seems fairly clear that the idea is that the header is NOT a source of truth, and therefore should silently be overriden; I agree that it could lead to very weird behavior (I've seen some demonic errors with headers before) in some cases, but if the standard is known, it also makes a lot of other behaviors easier - Like streaming data piece by piece and not even having to worry about whether the length indicated is right or not, because the truth comes from the data being sent, not an arbitrary length marker. And if you want to treat the web version of buffer overflow, you should be able to handle that server side, by inspecting the length of the data itself if necessary. Because it's the source of truth, and Header-Length is only an indicator.
That being said, do we really need that header then ? Probably not, but being compliant with old projects mean we often keep data that we don't really need anymore, even within standards.
@@Dekharen header IS the source of truth. When you get HTTP request you don’t know how much data goes other than from Content-Length headers. Without it you’d have to guess. And how would you guess? Based on some delimiter? What if that delimiter happens to show up in the data? Waiting for other party to stop transmitting? You’re transmitting over the network. Stuff happens, sometimes you need to wait for packet longer, sometimes shorter. Without Content-Length header HTTP would be terribly inefficient.
@@domints But that's the point. It makes the query more efficient but it's not considered the source of truth. It's only a reflection of the body. What I mean by that is that if your content is 8 bytes but your header says 10, there is not a single standard or library anywhere that would fill the object body with null characters to meet the length; and inversely, no one expects that if your header says 3, then the content should be shortened and cut to 3 bytes before being sent.
What you expect is for the response to be a mismatch message, for the header to automatically match, or for the code to simply throw.
Point being that the standards chose for the header to automatically match, and for you to verify whether that information is true on the server side. You can't just "trust" a header; it's totally possible to send a mismatched header, but iterating over the content length will always be the truth. And because (I'm guessing) it's strictly impossible to make the header "always" true, then it's just a convenience option that can sometimes fail.
There could be much more efficient ways to do this without http, but we handle things this way now.
@@Dekharen Allowing Content-Length to be set kind of dismisses what fetch is about: providing a HTTP-compliant client. I think domints points are valid, having Content-Length different from the actual amount of bytes sent would just be harmful to servers and not solve any type of problem.
I think the fetch API just overriding the Content-Length and not throwing any errors is a missed opportunity from the spec, and that unfortunately can't be changed now.
The two things most easy to get wrong in software are cache invalidation, the presence or lack of data, and off by one errors.
@@JeremyAndersonBoiseWhat are the other 02 types of programmer?
So, why not provide the ability to:
Provide a sunset date when a feature will be added/changed/removed and
Until then provide a compatibility layer for package maintainers to move to to allow for testing, validation, etc.
This would help with cases "the language cannot become more convenient because..."
A language cannot adapt and change without understanding how it is being used, and that requires a conversation between the group setting standards and the users of those standards.
If something is being used incorrectly and that use is widespread, it's better to correct that than to work around it later. 🤔
The fact that it's such a normal issue to have in Next, to fetch() the same data from the same source multiple times within a page render is insane.
Theo just goes straight into how deduping those requests is a good idea (duh), but without even mentioning how stupidly designed things become when every single "server component" fetches it's own data.
Also if I want to fetch the current permissions of a user, I want the *current* permissions, not what was there 10+ seconds ago. This is a recipe for dangerous race conditions.
I'd fetch once, set the data somewhere that lets me access it within the context of that request, without polluting fetch().
To the specific argument in the article, I agree fully. Patching globals is almost always a bad idea.
I think JavaScript is really the problem here. The fact that libraries can f up the behavior of other libraries is absurd. Just by installing one library with side-effects, all other libraries can malfunction is a design flaw of JavaScript and should be addressed.
I really like the deduping of requests in most cases. But: The issue with deduping is, that it still breaks certain behaviour. I‘m working on consuming a very old IBM Auth API that uses CSRFtokens. The deduping breaks that…
If you are an application, override all you want. If you are a library, please dont.
Can't people learn from mistakes and figure out that because of Mootools patching things we got such weird naming on array methods ?
To answer the "Why would you want the same call to have different results in the same render?", an example could be that you fetch "/userSuggestions" where it returns items for various places on the page. Think a video site and suggested videos, you wouldn't want all the places where video suggestions appear to be the same. Although this sort of request is the exception and would probably pass a UUID to ensure there is no caching.
if these frameworks don't like the behavior of the globals, why not implement their own version?
Ok I need to ask - why do we need auth requests ? Just make the user requests and catch a 401
I support this method.
Exactly - having an if authenticated then run x on the client side is just useless.
Scenario where you want the same fetch call during the same request to return different values: GET /next-card where the cards are random. In general, any call where the server is giving you random items from a collection or where it is randomly generating those items.
This resource should not be accessed through GET, as you are changing state on the server. GET should never change state on the server. You are performing a mutation, a shuffle and getting the result. So you should really be using POST, in which case deduping is not applied. You could also split it up into POST /shuffle and GET /last-card.
@@trombecher You are making assumptions. It is perfectly possible to ask for a random item without changing any state on the server. The prime example would be a provider of truly random values
@@kilburnsc Fair. But you have to differenciate between situations, where you actually want the same random value in various places on your site (like different layouts for multiple viewports, etc.) and like you mentioned multiple random requests, e.g. for a card game. I think we as developers should communicate that to the user, when deduping happens and how to prevent it.
@@trombecher `authenticate` (the main example from the video) is also a mutation of server state, so it shouldn't ever be a `GET` either.
@@Luxalpa You do not get around that + this primarily applies to the browser GETting HTML from the server + session invalidation happens quite seldom. We as developers expect middleware on locked down GET requests, which justifies this behaviour + you are not changing the state of your resource, but rather if and how you can access it.
"Our expectations as developers" is an emotional appeal which is utterly useless to anyone implementing anything. You still have no idea what "we" expect as developers, even after running a twitter poll that was answered by 2500 people on one platform. It's a bug in node. The professional annoyances of the Bun issues team aren't more important than the spec. Sorry.
My own problem with the library code argument is that you are assuming that library authors want fetch to be deduped or behave differently (especially for an SDK).
As a library maintainer myself, if I want to add deduping to the library, I'll add it myself. I'd rather not rely on Next's fetch overrides or Bun's overrides, etc, etc.
If I want to allow custom fetching behavior, I'd allow the user to pass in a fetcher during instantiation.
I don’t agree with your last take. Yes, I do expect epoch time to be consistent within the page, but imho, fetch API is not responsible for that. Fetch is the way to send http requests, it shouldn’t also be a caching function. It’s all about readability and intent, when overriding the function you introduce an unknown behavior without showing your intent, and make people do things they didn’t intend to. When writing a page that needs to call the same api 3 times and always get the same result, I’d try to make only one call and save the response to be reused. If I can’t do that, I’d reconsider what I’m doing and try to do it another way.
This is a convenience but it’s making your code less readable and less maintainable by someone else who may not know about your fetch override.
The fact that console log isn't reliable in next.js is one of the reasons I hate next. When I first started out as a dev I was always reaching for abstractions... now I want to know how everything works and make it myself. But that isn't always possible.
My biggest takeaway from this video was adding the word "footgun" to my vocabulary.
Regarding the import proxy, I think one advantage this has is you can override the global fetch call, and instead of just checking if options contains a property called proxy, you check if the resource is an instance of an object you own. That way if a new standard is made, it shouldn't clash.
I could also see a use for different responses, if there is rate limiting, so once you hit a certain number of requests, you get stopped.
If that check auth request is what is doing the rate limiting, then it shouldn't get cached.
that fetch content length thing should really just be undefined behaviour.
This is even worse than just picking one proposed behavior. IMO it should either error, or take the length from the content passed in.
@@trombecher if you try to do something that literally doesn't make sense in the first place... thats your fault.
@@joshuathomasbird Undefined behavior should not happen in JavaScript. It is better to inform the user of such UB with an Error. UB could also be a security vulnerability.
One thing I don't think was addressed in great length is that once you decide to patch, you now have to worry about backwards compatibility. If you decide to change your approach in the future, it will be very painful for those that leaned on your patched APIs. I think this may be one reason that IE was so bad to develop for. Microsoft made decisions before there were standards, and they had to maintain backwards compatibility when they started adopting standards. This is the nightmare that led to them deprecating IE and moving to Chromium. You don't want your framework or runtime to become the next IE.
the thing thats crazy to me is like just prefix stuff like bproxy for bun or whatever like what feature you add just make sure to jam a letter on it so you dont have issues
Deduping is caching Theo, it's just a more specific use-case.
The problem is not patching globals, it is having globals to begin with!
The problem is the ability to mutate globals, so that they effect other libraries. Static, immutable should be fine tho.
Wasn't this already uploaded?
Is there a reason React and Next can't just make useFetch() and nextFetch() functions? I don't understand why you need to override the global functions.
Does the NextJS cache behavior respect Cache-Control headers? (obviously deduping can't).
Honestly I hope the Date patch fixes my issue because I'm out of ideas. I'm trying to render created at date on the client (using drizzle as orm), and the results so far are complete failure. I get the data fine, I console log the date as instance of Date, but when I try to parse it to locale string I get NaN for some stupid reason. I tried date-fns to format it and it actually hard crashes my app. I'm stuck not knowing what to do, and retroactively adding a column to 600+ entries by hand with a valid date string is not viable.
Can someone explain wth is deduping, I understood upto 18:00 min mark, but then around 18:21 deduping confused me.
People not wanting their work to be used in a particular way doesn't mean other people wont. Especially if it is valid criticism.
I'm tired of the issues caused by GraphQL using POST. It might not be practical, but is it _possible_ to just not use _any_ of the HTTP methods? Or to add one called GRAPH so that it's not constrained by the HTTP POST method? It occurs to me that the GraphQL use of POST causes problems for everyone, not too different from the topic of this video.
Could Next alternatively clone the fetch api so that you’d call import { fetch } from "nextjs"? Then they could patch their clone of it all day long. Does the js runtime allow that, and would that solve these issues?
It's so funny to see JS have this debate _again_, when JS and other languages already had the monkey patching debate a decade ago, and p much everyone and their uncle agreed it's dangerous and should be used with the caution of the nuclear option it is.
can somebody give insight on why we would want users to set custom content length that does not match payload? I have heard of some bugs where modified content length leads into some bugs where api now spits back something from adjacent memory making that an exploit, so I would expect a throw is payload does not match declared length
A memory leak (as you described) is rather seldom, as JavaScript has automatic memory management and such a memory leak would be a bug in the implementation of Request. Other than that I agree: it should throw or just take the length from the content passed in.
@@trombecher I don't think I described it well, there was security vulnerability where server would send you more data than it was supposed to due to modified length. JS memory leaks ain't that bad, not concerned by those too much
i only just realized that this channel is doing react content in two ways lol
Want a full video on MSW
You and me both, friend!
18:43 - page 1 is okay maybe, page 2 is never okay. Reason why I say maybe on page 1, depneds on the time between each. When should we stop trusting the current cache?
23:00 - The problem sometimes it takes years to make good debuggers. Heck look at chrome, firefox and other tools that took 20+ years to be decent. Most likely will need most JS devs moving from something like VSC to Jetbrains
19:45 wait... you all aren't using GET on GET requests? Why?
Theo have you seen the announcement of the new Dev Tools from Vercel? Maybe they plan on implementing caching dev tools in there as well
> "It's just a library"
Internet Explorers XMHttpRequest would always cache GET requests, like, ALWAYS. Caused me no end of issues trying to work out what the hell was going on, especially since Firefox and Chrome at the time didn't exhibit the same behaviour.
My fix at the time, which I've continued to do 17yrs later is provide a 'time' parameter in the GET query string, which changes for every single request, because I do not want the caching at all.
But I don't use crap like Next.js or React, and never will.
for ruby on rails, this are so norm, monkey patching is like game for all gems creator, debugging that really really sucks
i think a patching platform alike middleware will be cool
Why use var or let if you can use window.
To reuse your language: I have no idea why anyone would think writing 4 identical fetch calls in one render is a good idea. God gave us variables and useContext, and component props for a reason. Use them. Writing the same request 4 times should send me the same request 4 times because you wrote shitty code that sends the same request 4 times. That's not a library's problem to solve.
In this case, i think you're overlooking a real risk, for example around volatile data.
have u looked into the roadmap (5yr-10yr) for Epic's "Verse" lang? They wanna make everything global.
Game modders know all-too-well the compatibility issues that come with "Patching things you don't own". It's all fun-and-games using the lua playground until some 1000iq breaks your mod because they patched a global XD
How about don't patch just create a library, some sort of std library for your framework...???
Yeah this is just bad design. Deduping isnt a problem if you build your application correctly
These videos are always so damn long 😭
I would 100% expect it to be 12, 13, 14. I would not expect it to be 12, 12, 12. But maybe that's the difference between us. I do business software development, for real world business related stuff that allows a business to sell stuff both at a retail store level and online.
From watching a lot of your videos, I've come to the conclusion you seem to be completely isolated from that sort of development.
It's bad and they should feel bad
i disagree. why not just create a separate fetch function then import that? it seems like the easiest solution to all of this mess
Please make a video on MSW!
Overrides are BAD! Don't override basic functionality, create an alternative!
well i think this channel is for juniors/mids that want to be leads and are probably going to...
Hey! What's the browser he is using?
Arc Browser
@@balazsmolnar5498 thanks bro!
import nextFetch from "next/fetch" - this makes all the justification for trying to patch globals INVALID, it's that simple, why bother patching? Insanity
JavaScript is such a piece of $!?% language. A proper language wouldn't allow you to do this. I wish the web could get off of JS and lean much heavier on WASM.
I agree on the JS side. But WASM is nothing without bindings to JS, C, etc. I read a large portion of the WASM spec for a side-project and all you can really do is some math and data transformations (fancy, I must add, SIMD, etc.) but that's about it. File IO, document access, all those things in which you interact with the environment do not exist. WASM relies on bindings to JS (which I also do not like, because you are dependent on JS). A better way would be to just invent another language and let it compile to JS for the web.
both Rust and C++ allow you to patch methods at runtime, it's just more complicated for them to do it. This is not a language issue, this is an issue with how you use it and which libraries you use.
@@Luxalpa I don’t know about c++, but in Rust a function you import from a library, or even a struct/function. The global scope cannot be patched/changed. If you import that item, sure, but then you are at fault because you imported that. In Rust it is impossible for one crate to change the environment/behavior for another crate.
@@Luxalpa So it is basically a language issue, as Rust does not permit such things.
Next isn't ECMAScript anymore. That's so sad. First, they wanted to own the server, now the runtime, and in the future, maybe the infrastructure.
At 41:35, why would you call the same fetch 3 times in a single page/load? It seems to me that the responsibility to handle, store the response and reuse it in 3 places have shifted from the client to the library. Which, at least for me, looks like too much babysitting, the client is the one with the business rule so the client should know how to handle it, fetch calls shouldn't care about it whatsoever.
Manual deduping of functions screws up DX. This is a common problem encountered in web dev, so common in fact that code would be unmaintainable if every site author implemented their own request deduping: they might as well have the horrendous idea of patching globals themselves.
Stop hacking around shit SDK and tell the authors to build it with good architecture pr fork it and make it better by yourself
great vid
nice video as always
Why does Theo love to be on the bad side of things? This is beyond clickbait
"This piece is not part of that backlash" maybe I should read the rest first but if this article is a criticism of frameworks patching then sorry Author, it is part of the backlash. Why would there be anything wrong with that anyway? If you have a criticism, don't mince your words by saying shit like that. He's just diluted his own point in the first paragraph. Either it is a criticism of these changes or it isn't. You can't say "yeah I thoroughly disagree with this and my way is better and i'm 100% criticising but i'm not actually criticising I'm everyone's friend I promise!!!!".
I literally reached out to Vercel and Bun, and a person from the committee has proof-read the article. The sentence was about the blind hate patching globals received. There's zero hate in my article. I like Vercel, I like Bun. It just so happened, I like the standards more. Chill.
Got a new senior developer went into our codebase and have put this code
if (environment.production) console.log = []
and took me a week to find out why is our custom logger not functioned as it used to, hahaha fuck senior developer title...