-
Notifications
You must be signed in to change notification settings - Fork 78
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
Revamp importing to enable bumping Undici, and fixing instanceof tests within the edge runtime #309
Conversation
🦋 Changeset detectedLatest commit: 34e540b The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
7312b2a
to
0e74bc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 💯 Left some nitpicks for now. Will look at the details later.
Co-authored-by: Kiko Beats <josefrancisco.verdu@gmail.com>
Co-authored-by: Kiko Beats <josefrancisco.verdu@gmail.com>
…s within the edge runtime (vercel#309) Co-authored-by: Kiko Beats <josefrancisco.verdu@gmail.com> Co-authored-by: Gal Schlezinger <gal@spitfire.co.il>
This is a big iteration on how the
@edge-runtime/*
suite of packages work, done specifically to bumpundici
as a dependency.Some context: we wanted to add WebSocket support for the edge-light runtime, and that requires bumping Undici. Unfortunately, the mechanism we used to import it previously didn't work anymore, as it uses
worker_threads
.Thankfully, we also had a great idea on how to fix
instanceof
tests so we can avoid using unnecessary polyfills and share Uint8Arrays between the main Node.js VM and the VM we create for the edge-light runtime.Previously, the following code didn't work:
This is due to the fact that every V8 VM has its own
Object
,RegExp
,Uint8Array
(etc) classes. We fixed it now, and objects created outside of the VM will now beinstanceof
the classes from inside the VM.This little change means that instead of loading modules within the VM, we can load them in the main Node.js program, and everything will work as intended: so we can upgrade Undici!
Since we still support older Node.js versions, we need to polyfill some stuff, which requires special handling for importing things. Eventually I believe we will be able to drop them.
I also didn't do any conditional logic of imports yet. I believe we can do that and offload to the native implementations when possible (like TextEncoder and TextDecoder, or even use the native undici implementation that exist for newer versions)
More noteworthy changes:
@edge-runtime/primitives
into a new module called@edge-runtime/integration-tests
, which, surprisingly, only runs integration tests. This fixes the circular dependency ofprimitives -> jest-environment -> primitives
, which was annoying. Primitives also requires to build before running the tests so it's not like we're making a worse iteration speed. These tests are using@edge-runtime/ponyfill
so they actually being tested both using the@edge-runtime/jest-environment
and the standard Node.js environment using the same code!@edge-runtime/primitives/*
is discouraged. Since there is a mandatory order of imports, and we don't use the standardrequire
to import the dependencies, for good and bad.undici
means that we now have thegetSetCookies
function. We still support thegetAll
to avoid breaking changse, but internally it usesgetSetCookies
.structuredClone
polyfill now supportsReadableStream
(as mentioned in MDN), because Undici uses it.WebSocket
constructor now! (resolves EC-617)closes #265