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

Revamp importing to enable bumping Undici, and fixing instanceof tests within the edge runtime #309

Merged
merged 50 commits into from
May 2, 2023

Conversation

javivelasco
Copy link
Member

@javivelasco javivelasco commented Apr 19, 2023

This is a big iteration on how the @edge-runtime/* suite of packages work, done specifically to bump undici 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:

const vm = new EdgeVM();
vm.context.MY_BINARY_ARRAY = new Uint8Array([1,2,3]);
assert(vm.evaluate(`MY_BINARY_ARRAY instanceof Uint8Array`));
assert(vm.evaluate(`MY_BINARY_ARRAY instanceof Object`));

// If the previous statements were correct, this failed:
assert(vm.evaluate(`{} instanceof Object`));
vm.context.MY_OBJ = {};
assert(vm.evaluate(`MY_OBJ instanceof Object`))

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 be instanceof the classes from inside the VM. :yesss:
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:

  1. I extracted the tests from @edge-runtime/primitives into a new module called @edge-runtime/integration-tests, which, surprisingly, only runs integration tests. This fixes the circular dependency of primitives -> 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!
  2. Importing @edge-runtime/primitives/* is discouraged. Since there is a mandatory order of imports, and we don't use the standard require to import the dependencies, for good and bad.
  3. Bumping undici means that we now have the getSetCookies function. We still support the getAll to avoid breaking changse, but internally it uses getSetCookies.
  4. The structuredClone polyfill now supports ReadableStream (as mentioned in MDN), because Undici uses it.
  5. We expose WebSocket constructor now! (resolves EC-617)

closes #265

@changeset-bot
Copy link

changeset-bot bot commented Apr 19, 2023

🦋 Changeset detected

Latest commit: 34e540b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@edge-runtime/jest-environment Minor
@edge-runtime/jest-expect Minor
@edge-runtime/node-utils Minor
@edge-runtime/primitives Minor
@edge-runtime/user-agent Minor
@edge-runtime/ponyfill Minor
@edge-runtime/cookies Minor
edge-runtime Minor
@edge-runtime/format Minor
@edge-runtime/types Minor
@edge-runtime/vm Minor

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

@vercel
Copy link

vercel bot commented Apr 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
edge-runtime ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2023 11:25am

@Schniz Schniz requested review from Kikobeats and nuta May 1, 2023 11:48
Copy link

@nuta nuta left a 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.

packages/primitives/src/primitives/fetch.js Show resolved Hide resolved
packages/primitives/package.json Outdated Show resolved Hide resolved
docs/pages/packages/format.mdx Outdated Show resolved Hide resolved
docs/pages/packages/node-utils.mdx Outdated Show resolved Hide resolved
docs/pages/packages/ponyfill.mdx Outdated Show resolved Hide resolved
docs/pages/packages/primitives.mdx Outdated Show resolved Hide resolved
packages/vm/src/edge-vm.ts Outdated Show resolved Hide resolved
packages/integration-tests/package.json Outdated Show resolved Hide resolved
packages/integration-tests/package.json Outdated Show resolved Hide resolved
packages/integration-tests/package.json Outdated Show resolved Hide resolved
packages/primitives/src/primitives/fetch.js Show resolved Hide resolved
Co-authored-by: Kiko Beats <josefrancisco.verdu@gmail.com>
Co-authored-by: Kiko Beats <josefrancisco.verdu@gmail.com>
@Schniz Schniz merged commit ed225b3 into main May 2, 2023
@Kikobeats Kikobeats deleted the wip-patch-instanceof branch May 2, 2023 11:30
@github-actions github-actions bot mentioned this pull request May 2, 2023
@github-actions github-actions bot mentioned this pull request May 29, 2023
jridgewell pushed a commit to jridgewell/edge-runtime that referenced this pull request Jun 23, 2023
…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>
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.

undici v5.14.0 or higher is failing
4 participants