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

🐛 BUG: wrangler or miniflare turns on --experimental compatibility flag by default #2518

Open
IgorMinar opened this issue May 30, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@IgorMinar
Copy link
Contributor

IgorMinar commented May 30, 2024

Which Cloudflare product(s) does this pertain to?

Wrangler core

What version(s) of the tool(s) are you using?

cloudflare/workers-sdk#5878

What version of Node are you using?

22

What operating system and version are you using?

Mac

Describe the Bug

Observed behavior

I'm able to use the nodejs_compat_v2 experimental flag without having to turn on the experimental flag.

When logging the args (at this location) that workerd is called with, I see:

[
  'serve',
  '--binary',
  '--experimental',
  '--socket-addr=entry=127.0.0.1:0',
  '--external-addr=loopback=127.0.0.1:64059',
  '--control-fd=3',
  '-',
  '--inspector-addr=localhost:64055'
]

Expected behavior

The experimental mode should not be on by default and should require the application developer to opt into it.

Steps to reproduce

Please provide the following:

  • hello world app with workerd 1.20240529.0 and nodejs_compat_v2 flag turned on in wrangler toml

This should fail because the nodejs_compat_v2 flag is experimental. To fix this the user is expected to add experimental to the compatibility_flags array in wrangler.toml.

Please provide a link to a minimal reproduction

No response

Please provide any relevant error logs

No response

@IgorMinar IgorMinar added the bug Something isn't working label May 30, 2024
@IgorMinar
Copy link
Contributor Author

Fixing this will likely be a breaking change, so we should understand when this behavior was introduced (so that we can estimate the impact) and at minimum release the fix in a minor version.

@IgorMinar
Copy link
Contributor Author

IgorMinar commented May 30, 2024

btw I tested the nodejs_compat_v2 flag with raw workerd and it properly failed without the experimental mode opt-in:

./bazel-bin/src/workerd/server/workerd serve samples/nodejs-compat/config.capnp

service main: The compatibility flag nodejs_compat_v2 is experimental and may break or be removed in a future version of workerd. To use this flag, you must pass --experimental on the command line.

@mrbbot
Copy link
Contributor

mrbbot commented May 30, 2024

Hey! 👋 Unfortunately, --experimental is required by some compatibility flags Miniflare uses to enable local-only functionality. For example, service_binding_extra_handlers is required to allow calling Fetcher#scheduled()/Fetcher#queue(), but is marked "experimental" in workerd because we don't want it to be publicly accessible.

https://github.com/cloudflare/workers-sdk/blob/9a804b9061006f2c2d508151d86246400055e4fd/packages/miniflare/src/plugins/core/index.ts#L751

serviceBindingExtraHandlers @28 :Bool
$compatEnableFlag("service_binding_extra_handlers")
$experimental;

In theory, we could enable these with some other command line flag if needed, but enabling --experimental locally seemed ok, as you still have to opt-in to experimental features with the specific compatibility flag.

@IgorMinar
Copy link
Contributor Author

enabling --experimental locally seemed ok, as you still have to opt-in to experimental features with the specific compatibility flag.

the problem is that as a user right now, if I add nodejs_compat_v2 to wrangler.toml, I might not realize that I'm opting into an experimental functionality as there is nothing visible to me that indicates "experimental" mode.

For this reason, we decided to for now to prefix nodejs_compat_v2 with a mandatory experimental: prefix in wrangler only (wrangler will strip this before passing it on to workerd).

Ultimately however, I think that we should build the support for experimenta: prefix in workerd as we discussed in the past.

@penalosa
Copy link
Contributor

penalosa commented Jun 5, 2024

Linking to give context to future readers: cloudflare/workers-sdk#2534

@petebacondarwin
Copy link
Contributor

I don't think we can resolve this only in Wrangler. Instead this is more of a feature request for workerd. So I'm going to transfer the issue there to be triaged.

@petebacondarwin petebacondarwin transferred this issue from cloudflare/workers-sdk Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants