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

feat: add service bindings support #683

Conversation

danielrs
Copy link
Contributor

@danielrs danielrs commented Mar 23, 2022

Soon service bindings won't be experimental, this PR adds service bindings support to Wrangler 2.

Adds warning to unsafe bindings that were used as "raw" service bindings

Background

Experimental service bindings

Link: #166
Initial take on service bindings, replaced by the more generic unsafe bindings below.

Unsafe bindings

Link: #411
Used for any binding type not officially supported by Wrangler, including service bindings until today.

@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2022

🦋 Changeset detected

Latest commit: fabaf8b

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

This PR includes changesets to release 1 package
Name Type
wrangler 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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 23, 2022

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/2031004280/npm-package-wrangler-683

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

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/683/npm-package-wrangler-683

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2031004280/npm-package-wrangler-683 dev path/to/script.js

@danielrs danielrs force-pushed the drivas/feat-service-bindings-support branch from 2f28213 to 59b803e Compare March 23, 2022 21:58
@danielrs danielrs force-pushed the drivas/feat-service-bindings-support branch from 59b803e to fabaf8b Compare March 23, 2022 22:03
@threepointone
Copy link
Contributor

Let's use "name" instead of binding. Also, does this work with wrangler dev?

"wrangler": patch
---

feat: add service bindings support
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a description of what service bindings are, and a short example of what a service configuration in wrangler.toml would look like.

@petebacondarwin
Copy link
Contributor

Let's use "name" instead of binding. Also, does this work with wrangler dev?

I worry about the use of name. It is very generic and could be misinterpreted. In this case there are two names:

  • the identifier used in the code to refer to the service being bound
  • the name of the Service (i.e. Worker) that is being bound

I appreciate that it would be better if all these bindings were more uniform, but I am not sure that using name is the right answer.

@threepointone
Copy link
Contributor

I'd double down and say that "service" should be called "script_name"
"binding" is a weird cloudflare word and I can't think of many other things in the ecosystem called a binding
In the future (ideally when we have the toml editing thing) we should make them all consistent; for kv_namespaces, vars, etc

@mrbbot
Copy link
Contributor

mrbbot commented Mar 24, 2022

I'd argue script_name is more confusing. In my mind, a script is a single file, so I'd expect it to be a file path. I know this is what Durable Objects use, and they're called scripts internally, but service conveys exactly what you're pointing to.

As for binding, I think this is fine as the feature is called and has been announced as "Service Bindings". Also, as you say, "binding" is already used extensively throughout Workers. Potentially a more JavaScript'y name could be key as it's the key into the env object?

@threepointone
Copy link
Contributor

I don't feel super strongly about this since it's not consistent anyway across the different types, and don't want to block the PR. I'm more curious whether this works with wrangler dev, that would be amazing. @danielrs can you confirm?

@danielrs
Copy link
Contributor Author

Hey @threepointone ,

This doesn't work with wrangler dev just yet, as there's some ongoing work in the back-end that needs to be completed first (and should be completed before GA).

Binding name ambiguity

As for the name of the bindings, I agree that using name could be ambiguous. In this cases I think consistency is more important, by looking at the other bindings I observed there's mainly two different conventions being used.

The first one used by kv_namespaces and r2_buckets consists of each property being an array; they use binding for the name of the binding in the worker.

The second one used by durable_objects and unsafe consists of each property not being an array, instead they contain abindings array that has all the bindings; they use name for the name of the binding in the worker, but in this case the ambiguity is not an issue since it's under "bindings".

We could make the service bindings be one or the other, what do you all think?

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Mar 24, 2022

I'd double down and say that "service" should be called "script_name" "binding" is a weird cloudflare word and I can't think of many other things in the ecosystem called a binding In the future (ideally when we have the toml editing thing) we should make them all consistent; for kv_namespaces, vars, etc

Reading the comment in the code bound service why not call it bound_service/service_binding or service_name

@threepointone
Copy link
Contributor

I'm not comfortable landing this PR until it works for wrangler dev, it feels incomplete until then. Folks can still use unsafe.bindings if they want it before that.

@threepointone
Copy link
Contributor

Any updates here @danielrs?

@threepointone
Copy link
Contributor

Closing this in favour of #906

mrbbot added a commit that referenced this pull request Oct 31, 2023
When running in Jest, `require("undici")` inside a worker thread
fails. Instead, we need to call `createRequire()` first, using the
`__filename` of the worker host. The `require` function returned
from that can then be used to `require("undici")`.
mrbbot added a commit that referenced this pull request Nov 1, 2023
When running in Jest, `require("undici")` inside a worker thread
fails. Instead, we need to call `createRequire()` first, using the
`__filename` of the worker host. The `require` function returned
from that can then be used to `require("undici")`.
mrbbot added a commit that referenced this pull request Nov 1, 2023
When running in Jest, `require("undici")` inside a worker thread
fails. Instead, we need to call `createRequire()` first, using the
`__filename` of the worker host. The `require` function returned
from that can then be used to `require("undici")`.
mrbbot added a commit that referenced this pull request Nov 1, 2023
When running in Jest, `require("undici")` inside a worker thread
fails. Instead, we need to call `createRequire()` first, using the
`__filename` of the worker host. The `require` function returned
from that can then be used to `require("undici")`.
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.

5 participants