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

Add support for text_blobs #228

Merged
merged 5 commits into from Mar 30, 2022
Merged

Add support for text_blobs #228

merged 5 commits into from Mar 30, 2022

Conversation

caass
Copy link
Contributor

@caass caass commented Mar 30, 2022

I have absolutely no idea what I'm doing here 😎

Opening in hope of closing #211

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty bang on to me! 🙂Added a few comments. If you could add some tests here too that would be awesome: https://github.com/cloudflare/miniflare/blob/master/packages/core/test/plugins/bindings.spec.ts.

packages/core/src/plugins/bindings.ts Outdated Show resolved Hide resolved
packages/core/src/plugins/bindings.ts Outdated Show resolved Hide resolved
packages/core/src/plugins/bindings.ts Outdated Show resolved Hide resolved
packages/core/src/plugins/bindings.ts Outdated Show resolved Hide resolved
packages/core/src/plugins/bindings.ts Outdated Show resolved Hide resolved
Comment on lines 326 to 328
textBlobBindings: {
D: loremIpsumPath,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with this test is that the lowest priority binding defines the most bindings, that are then overriden by subsequent higher priority bindings. For textBlobBindings, you'd want to define A, B, C as loremIpsumPath, for wasmBindings you'd define A, B, C, D, for envPath you'd define A=env\nB=env\nC=env\nD=env\nE=env and then for wrangler bindings you'd define A, B, C, D, E, F.

This is probably a little excessive, but it's nice to test every combination 🙂

@mrbbot mrbbot merged commit 57dd23d into cloudflare:master Mar 30, 2022
@mrbbot mrbbot mentioned this pull request Mar 30, 2022
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Mar 31, 2022
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
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Mar 31, 2022
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
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Mar 31, 2022
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
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Mar 31, 2022
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
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Mar 31, 2022
…ode (#735)

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
@mrbbot mrbbot added this to the 2.4.0 milestone Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants