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

We should error when a user tries to implement a durable object in a worker using the service-worker format #635

Closed
Tracked by #619
caass opened this issue Mar 18, 2022 · 0 comments · Fixed by #640

Comments

@caass
Copy link
Contributor

caass commented Mar 18, 2022

From @Electroid:

There is a little more nuance: modules are required to implement a Durable Object, but not to bind to one. Therefore, it’s only an error when the format is service worker and the Durable Object binding does not have a “script_name”.

I tried a quick implementation (#634) but actually I think this is maybe more complex than a simple fix like that.

We currently guess the worker format by checking for exports in the bundled code generated by esbuild. If there are no exports, then the worker is service-worker format. If there are some, it's a module worker.

The issue I'm running into is that a Durable Object implementation will contain an export, necessarily. So checking for the existence of exports is kind of not enough to cover this case, cause what if the user writes code like the following:

export class DurableObjectExample {
    constructor(state, env) {
    }

    async fetch(request) {
        return new Response('Hello World');
    }

}

// omitted: wrangler.toml binds DurableObjectExample as EXAMPLE_DO

addEventListener('fetch', event => {
  event.respondWith(handleRequest(event.request));
});

async function handleRequest(request) {
  // env is just set up as a global in service workers
  const id = EXAMPLE_DO.idFromName(request.url);
  const stub = EXAMPLE_DO.get(id);
  return stub.fetch(request);
}

Edit:

I have been informed that by virtue of having an export statement, this code is actually an ESModule, and will error with "no default export" or some such when attempting to publish. So it's actually reasonable to keep the current logic, and just check if there's a script_name as Ashcon suggested.

@caass caass changed the title We should error when trying durable objects from service-worker format We should error when a user tries to implement a durable object in a worker using the service-worker format Mar 18, 2022
threepointone pushed a commit that referenced this issue Mar 22, 2022
Durable Objects [require Module Workers](https://developers.cloudflare.com/workers/learning/migrating-to-module-workers/#advantages-of-migrating) to function, so we should error if we discover that a user is trying to use a durable object with a Service Worker.

Closes #635
mrbbot added a commit that referenced this issue Oct 31, 2023
* Bump to `[email protected]` and `@microsoft/[email protected]`

TypeScript 5 includes lots of goodies, and allows us to remove a
bunch of `@ts-expect-error`s.

* Set `"moduleResolution": "bundler"` in `tsconfig.json`

This loosens some resolution rules in accordance with how bundlers
like `esbuild` resolve modules. This also allows us to remove a bunch
of `@ts-expect-error`s.

* Remove import cycle in `src/plugins/r2`

This probably wasn't a problem, but generally it's not great to have
cyclic dependencies. For future reference, this was found with
`npx madge --circular --extensions ts packages/miniflare/src`.
mrbbot added a commit that referenced this issue Nov 1, 2023
* Bump to `[email protected]` and `@microsoft/[email protected]`

TypeScript 5 includes lots of goodies, and allows us to remove a
bunch of `@ts-expect-error`s.

* Set `"moduleResolution": "bundler"` in `tsconfig.json`

This loosens some resolution rules in accordance with how bundlers
like `esbuild` resolve modules. This also allows us to remove a bunch
of `@ts-expect-error`s.

* Remove import cycle in `src/plugins/r2`

This probably wasn't a problem, but generally it's not great to have
cyclic dependencies. For future reference, this was found with
`npx madge --circular --extensions ts packages/miniflare/src`.
mrbbot added a commit that referenced this issue Nov 1, 2023
* Bump to `[email protected]` and `@microsoft/[email protected]`

TypeScript 5 includes lots of goodies, and allows us to remove a
bunch of `@ts-expect-error`s.

* Set `"moduleResolution": "bundler"` in `tsconfig.json`

This loosens some resolution rules in accordance with how bundlers
like `esbuild` resolve modules. This also allows us to remove a bunch
of `@ts-expect-error`s.

* Remove import cycle in `src/plugins/r2`

This probably wasn't a problem, but generally it's not great to have
cyclic dependencies. For future reference, this was found with
`npx madge --circular --extensions ts packages/miniflare/src`.
mrbbot added a commit that referenced this issue Nov 1, 2023
* Bump to `[email protected]` and `@microsoft/[email protected]`

TypeScript 5 includes lots of goodies, and allows us to remove a
bunch of `@ts-expect-error`s.

* Set `"moduleResolution": "bundler"` in `tsconfig.json`

This loosens some resolution rules in accordance with how bundlers
like `esbuild` resolve modules. This also allows us to remove a bunch
of `@ts-expect-error`s.

* Remove import cycle in `src/plugins/r2`

This probably wasn't a problem, but generally it's not great to have
cyclic dependencies. For future reference, this was found with
`npx madge --circular --extensions ts packages/miniflare/src`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant