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 email worker and send email support #624

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LuisDuarte1
Copy link

Closes #274

Adds Email Worker support in workers-rs in addition to Send Email support

@LuisDuarte1 LuisDuarte1 changed the title Add email worker and send emailsupport Add email worker and send email support Aug 23, 2024
@LuisDuarte1 LuisDuarte1 force-pushed the lduarte/email-support branch 2 times, most recently from 15deda7 to d639b16 Compare August 23, 2024 09:13
@LuisDuarte1 LuisDuarte1 force-pushed the lduarte/email-support branch 2 times, most recently from 75e3e45 to a72c76b Compare August 26, 2024 15:49

// For some reason, email events doesn't seem to use WorkerEntrypoint so we get the env and ctx from
// from the function itself.
async email(message, _env, _ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

@ObsidianMinor do you know why email worker wouldn't use the new interface? Will cron triggers have the same issue?

Copy link
Author

Choose a reason for hiding this comment

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

I still haven't figured it out yet - for some reason env is still not probably propagated into the rust layer, and therefore I can't seem to use bindings with it 😅

Copy link
Author

@LuisDuarte1 LuisDuarte1 Aug 27, 2024

Choose a reason for hiding this comment

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

Its also quite difficult to debug because email workers can't be run locally 😅


// TODO(lduarte): see if also accepting string is needed
#[wasm_bindgen(constructor, catch)]
pub fn new(from: &str, to: &str, raw: &str) -> Result<EmailMessage, JsValue>;
Copy link
Member

Choose a reason for hiding this comment

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

Should we also have a new_from_stream method that takes ReadableStream?

}

#[wasm_bindgen]
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add headers field? Should these getters also be on EmailMessage?

pub fn forward(
this: &ForwardableEmailMessage,
rcpt_to: &str,
headers: Headers,
Copy link
Member

Choose a reason for hiding this comment

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

This field should probably be optional, or maybe have forward and forward_with_headers. Or is there no difference between headers being undefined or {} in JavaScript API?

use wasm_bindgen::JsCast;
use wasm_bindgen::JsValue;
use wasm_bindgen_futures::JsFuture;
use worker_sys::EmailMessage as EmailMessageExt;
Copy link
Member

Choose a reason for hiding this comment

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

I would typically call this EmailMessageSys.

}

pub async fn reply(&self, message: EmailMessage) -> Result<(), JsValue> {
JsFuture::from(self.0.reply(message.0)?).await?;
Copy link
Member

Choose a reason for hiding this comment

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

You may want to wrap these async calls as follows, otherwise the method's Future will not be Send:

let promise = self.0.reply(message.0)?;
let fut = SendFuture::new(JsFuture::from(promise));
fut.await?;

Basically, you don't want to hold JsFuture across await boundary.

unsafe impl Send for EmailMessage {}
unsafe impl Sync for EmailMessage {}

impl JsCast for EmailMessage {
Copy link
Member

Choose a reason for hiding this comment

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

I may be wrong, but you may not need all of this boilerplate for types that do not need to implement EnvBinding.

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.

[Feature] Add email worker support
2 participants