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

new event info type email #249

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

edevil
Copy link
Contributor

@edevil edevil commented Jan 5, 2023

Added new event info type for email events. This will contain the from, to and raw size of the email.

src/workerd/api/trace.h Outdated Show resolved Hide resolved
@harrishancock
Copy link
Collaborator

@jasnell, unrelated to this specific PR, but should the prototype property change (#249 (comment)) be extended to all the other similar EventInfo types, potentially with a compatibility flag?

@jasnell
Copy link
Member

jasnell commented Jan 5, 2023

... but should the prototype property change (#249 (comment)) be extended to all the other similar EventInfo types, potentially with a compatibility flag?

We should likely start transitioning those, yes, but not sure how much of a priority it is.

Added new event info type for email events. This will contain
the from, to and raw size of the email.

private:
kj::Own<Trace> trace;
const Trace::EmailEventInfo& eventInfo;
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is a new problem at all but I do generally have a concern about using const refs in jsg::Objects like this. What guarantee do we have that the ref is going to stay alive for as long as the JS wrapper? For instance, if someone happens to store the info object at global scope, for instance, what happens when the properties are accessed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, we should look at this.

@harrishancock harrishancock merged commit a5af378 into cloudflare:main Jan 6, 2023
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.

3 participants