RemixJS compatibility issues

[tygern]

I see an error that looks like

service core:user:remix-cloudflare-workers: Uncaught ReferenceError: Buffer is not defined

when I add instrumentation to a Remix app. I've created a minimal example to reproduce. I see this error whether or not I use the node_compat flag in wrangler.toml. Any help would be appreciated.

[evanderkoogh]

First thank you for reminding me that I needed to add the node_compat flag to the README :)

I can't think of a good reason why Remix would include Buffer when you add the library? I will try to find some time later today to have a quick look at it. Thanks for creating that minimal repro.. Impossible to overstate how much of a difference that makes to people maintaining packages :)

[evanderkoogh]

Ugg.. this is an ugly hack by open-telemetry that we run into. They are using /platform/node/* & /platform/browser/* in certain packages and use the package.json browser field to overwrite the node version with the browser version.

The standard wrangler bundler probably picks the browser option, but Remix probably uses the node one.

If you can make that switch in Remix, that would be a (albeit shitty) workaround for now. I am going to have a think on how best to solve this seamlessly, so that things will work no matter what your bundler picks for you.

[evanderkoogh]

Hmm.. that fix was easier than I thought and slightly uglier than I had hoped..

Cloudflare Workers have a working implementation for Buffer, but it is not in the global scope, but after an import and setting it on the global scope, your repro worked.

Can you confirm that it works in your full example too?

[tygern]

Thanks for tracking that down so quickly! I'll check the fix out on our full codebase and let you know how it goes.

[tygern]

Looks like we made some progress. We're able to build now without the buffer error. However, when we visit the root route we get another error from miniflare regarding the async_hooks polyfill:

[mf:err] Error: Node.js async_hooks module is not supported by JSPM core outside of Node.js
    at new unimplemented (/Users/initialdev/workspace/buffer-issue/build/node-modules-polyfills:node:async_hooks:3:9)
    at new AsyncLocalStorageContextManager (/Users/initialdev/workspace/buffer-issue/node_modules/@microlabs/otel-cf-workers/src/context.ts:227:29)
    at WorkerTracerProvider.register (/Users/initialdev/workspace/buffer-issue/node_modules/@microlabs/otel-cf-workers/src/provider.ts:36:35)
    at init (/Users/initialdev/workspace/buffer-issue/node_modules/@microlabs/otel-cf-workers/src/sdk.ts:78:12)
    at null.<anonymous> (/Users/initialdev/workspace/buffer-issue/node_modules/@microlabs/otel-cf-workers/src/sdk.ts:131:4)
    at Object.apply (/Users/initialdev/workspace/buffer-issue/node_modules/@microlabs/otel-cf-workers/src/instrumentation/fetch.ts:141:19)
    at proxyHandler.apply (/Users/initialdev/workspace/buffer-issue/node_modules/@microlabs/otel-cf-workers/src/wrap.ts:27:19)
    at __facade_modules_fetch__ (/private/var/folders/j0/0hw1hwyd365809kbgr9ccwm80000gn/T/tmp-14615-AXXR2YV3ee20/middleware-loader.entry.ts:46:16)
    at __facade_invokeChain__ (/Users/initialdev/workspace/buffer-issue/node_modules/wrangler/templates/middleware/common.ts:53:9)
    at Object.next (/Users/initialdev/workspace/buffer-issue/node_modules/wrangler/templates/middleware/common.ts:50:11)

I updated the example repo, so you should be able to reproduce the issue there. Thanks!

[evanderkoogh]

It is way past my bedtime here, but I had a quick scan and the error is present in the build artifact in build/index.js, which would suggest that Remix does some magic bundling with NodeJS polyfills. Which they shouldn't be doing (anymore?)..

I'll try to figure out tomorrow if it is Remix proper because of browser support or if it is the Cloudflare Workers adapter..
But it doesn't seem to be a bug in this library. But let's keep the issue open to track Remix compatibility.

[tygern]

Sounds good. I agree with you that the issue is likely with how RemixJS is bundling. I'll do a bit of digging there and will reply with any findings.

[tygern]

Looks like there's a relatively new Remix issue regarding this remix-run/remix#6715

[evanderkoogh]

That is indeed the issue you are seeing. Mark happens to be a friend of mine. I'll see if I can get in touch with him today or next week to have a chat about it :)

[evanderkoogh]

Ok.. had a long conversation with Mark about Remix and polyfills and how they can best handle this situation. The current plan is for you to have the ability to opt-into exactly the polyfills that you want. That should allow you to opt-out of the async_hooks polyfill.

He will be keeping the issue on the Remix side updated with a PR and progress once there is more to share.

[tygern]

Sounds like a good solution. Glad you’re well connected 😄

[tygern]

Looks like the fix in remix-run/remix#6814 gets things working for us. We've updated the buffer-issue repo to use the nightly mentioned in the PR. The one clunky bit is that opentelemetry requires child_process to build, which is one of the unimplemented polyfills excluded from the Remix build by default. This means that we have to configure Remix to add child_processes and several other polyfills to the build. Luckily, child_process doesn't seem to be used at runtime, so telemetry works as expected.

[evanderkoogh]

Well.. good to see that the fix works at least. But I am quite surprised that opentelemetry requires all these Node dependencies?! Would be worth investigating where that goes wrong. If I grab the buffer-issue repo and remove the polyfills, that should make it fail in the way that you saw right?

[tygern]

We were surprised too! Yes, if you set serverNodeBuiltinsPolyfill: true in the buffer-issue repo you'll see an error about child_process being missing when running npm run build. Once you add serverNodeBuiltinsPolyfill: ['child_process'] you'll see a bunch of other errors for missing node libraries on build.

[tygern]

Looks like the 1.19.0 Remix release fixes this issue. I'll close for now. I appreciate your help!