Check if middleware is an array

[penalosa]

What this PR solves / how to test

Use the named export __INTERNAL_WRANGLER_MIDDLEWARE__ for listing the middleware to be injected in module-format workers. To inject middleware for testing, use __INJECT_FOR_TESTING_WRANGLER_MIDDLEWARE__.

This removes support for adding a middleware array to your exported handler.

Fixes #5420.

Author has addressed the following

• Tests
  • TODO (before merge)
  • Included
  • Not necessary because:
• Changeset (Changeset guidelines)
  • TODO (before merge)
  • Included
  • Not necessary because:
• Public documentation
  • TODO (before merge)
  • Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/...
  • Not necessary because: not a documented feature

[changeset-bot]

🦋 Changeset detected

Latest commit: 1b189a1

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Patch
@cloudflare/vitest-pool-workers	Patch

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

[petebacondarwin]

So this means that it won't crash for itty-router anymore.
But do we actually want to ignore middleware that has been proxied such as is happening there?

[penalosa]

I believe we do, yes. As far as I know we've never documented this middleware behaviour, and I think we implemented it for testing purposes.

[petebacondarwin]

I don't think it is just for testing... we add these middlewares to handle specific wrangler dev scenarios such as the __scheduled path handling. See https://github.com/cloudflare/workers-sdk/tree/main/packages/wrangler/templates/middleware

If the itty-router is just wrapping the array containing these middlewares in a proxy that is then ignored by our bundler then these middlewares are no longer applied and so this functionality would be unavailable to them.

[petebacondarwin]

Shouldn't the fix be for the itty-router to pass through this middleware property untouched by the Proxy that they create?

[penalosa]

Sorry, that wasn't super clear—we do definitely use middleware, but I think this way of defining middleware (a user specified array in the exported handler object) was only for testing?

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-wrangler-5703

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5703/npm-package-wrangler-5703

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-wrangler-5703 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-create-cloudflare-5703 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-cloudflare-kv-asset-handler-5703

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-miniflare-5703

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-cloudflare-pages-shared-5703

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9212466448/npm-package-cloudflare-vitest-pool-workers-5703

Note that these links will no longer work once the GitHub Actions artifact expires.

---

wrangler@3.57.1 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20240512.0
workerd	1.20240512.0	1.20240512.0
workerd --version	1.20240512.0	2024-05-12

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[petebacondarwin]

As discussed offline. I wonder if we should avoid reading the middleware property from the default export at all? Perhaps move it to a non-default export?
I believe we could do this if this is only an internal API...

[kwhitley]

Shouldn't the fix be for the itty-router to pass through this middleware property untouched by the Proxy that they create?

We don't actually include/expose a middleware property at all. Our router is an object with a Proxy prototype. Any attributes on the object are accessed normally, and unknown attributes are intercepted by the proxy, interpreting them as route handlers.

So a accessing router.middleware returns a function like you might use here:

router.middleware('/path', () => 'this will match the MIDDLEWARE HTTP verb...')

As a result, something like this (below) will throw a similar error to what we're seeing here, as it DOES find router.middleware, but it's a function... not an iterable array:

for (const handler of router.middleware) {
  // do something
}

[kwhitley]

Any update on this?

We'd love to remove our disclaimer for using itty with the very environment it was designed for! :)

[penalosa]

As discussed offline. I wonder if we should avoid reading the middleware property from the default export at all? Perhaps move it to a non-default export? I believe we could do this if this is only an internal API...

This should be ready for a re-review now

[petebacondarwin]

Love it!

[petebacondarwin]

NIT: I would prefer to put the join(",") here rather than in the variable declaration above.