-
Notifications
You must be signed in to change notification settings - Fork 741
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
text_blobs
/Text module support for service worker format in local mode
#735
Conversation
🦋 Changeset detectedLatest commit: fe45b91 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2071874711/npm-package-wrangler-735 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/735/npm-package-wrangler-735 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2071874711/npm-package-wrangler-735 dev path/to/script.js |
f8b8f4f
to
ec32e8e
Compare
Don't land this just yet, I'm doing some manual testing here. Please review tho! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. We'll wait to see the result of the manual testing before merging.
} else if (module.type === "buffer") { | ||
throw new Error( | ||
'The "buffer" module type is not yet supported in service worker format workers' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change specific to this PR? It kind of feels like it is an independent fix? But I might be misunderstanding its importance in terms of text_blobs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just rounds out the story for module system support. With module workers, we can import modules of all the supported types: js, wasm, text, and data. With service-workers, we can import js, wasm, text, but not data. We can detect its usage and throw an error. I simply moved the error to earlier in the pipeline, right when we're building the worker, instead of waiting for when we create the worker form upload.
I don't expect this error to trigger often; there are no service-worker workers right now that import a Data module (because there's no equivalent like wasm_modules
or text_blobs
for data.) The usecase this will probably trigger is when a third party library imports a Data module, and is used in a service-worker (It'll work fine with a module worker). If that happens, and it's a good/common library, then the thing to do will be to convince the runtime team to add a new type of binding (data_buffers
?). We can then leverage that to add support for Data modules for service-workers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked internally, and we do have support for a data_blob
binding. Filed #740
Ok, so I tested this as thoroughly as I could. I was specifically checking whether existing usecases would break now that we're passing an unrecognised argument to the miniflare api. It simply ignores it, and results in an error in local mode when used in a service worker (as expected). I checked with combinations of module/service-worker+imports/global bindings, with different module types. Nothing that currently works appears to have broken, which is good. We can land this, and when miniflare does a release, I'll do another round of manual tests and it should all just work. |
This adds support for `text_blobs`/Text module support in local mode. Now that cloudflare/miniflare#228 has landed in miniflare (thanks @caass!), we can use that in wrangler as well. Fixes #416
ec32e8e
to
fe45b91
Compare
This adds support for
text_blobs
/Text module support in local mode. Now that cloudflare/miniflare#228 has landed in miniflare (thanks @caass!), we can use that in wrangler as well.Fixes #416
We can land this before miniflare does a release, but it won't work until we update miniflare. // cc @mrbbot