-
Notifications
You must be signed in to change notification settings - Fork 249
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 Workers Ai fetcher api #1877
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
1316266
to
ac5537f
Compare
1cf3151
to
fa31f6e
Compare
fa31f6e
to
73eb741
Compare
More tests seem worthwhile here? |
( | ||
name = "ai", | ||
wrapped = ( | ||
moduleName = "cloudflare-internal:ai-api", |
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.
hmm, internal
implies it shouldn't be used by end users. But this isn't the case here, right? Maybe this shouldn't be an internal module?
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.
humm, but end users shouldn't be able to initialize the Ai class on themself, it should be done by wrangler automatically when they define the AI binding
I've kept the code in the internal folder, but added a mapping outside to end users could import the options and Ai class from cloudflare:ai
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.
What do other APIs do? What is the pattern we want people to use for this @mikea ?
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.
hmm, internal implies it shouldn't be used by end users.
cloudflare-internal:ai-api
should indeed not be importable by end user.
In the case of a wrapped binding the module must be an internal one. See here:
https://github.com/cloudflare/workerd/blob/main/src/workerd/server/workerd.capnp#L508
The idea is that config is not managed by application developer, but by service provider, so that is a protection from application developer messing with bindings from code.
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.
2dccee5
to
285b95c
Compare
@irvinebroque i've just added more tests now |
285b95c
to
63800af
Compare
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.
code (and test) structure LG to me. I don't have much opinion on the API itself or its implementation detail.
Was able to successfully test this pull request together with the wrangler pull request here and i've confirmed that it is working as expected |
Can we merge this so that it can go out in a workerd release for Wrangler, and then EW? |
Everything ready from my side |
No description provided.