-
Notifications
You must be signed in to change notification settings - Fork 9
Add wshim support #167
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 wshim support #167
Conversation
d4370ab to
67ad8cf
Compare
67ad8cf to
67cd40b
Compare
cjpatton
left a comment
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.
Noooicce! Just one minor comment.
@rozbb can you test out ct_worker? I've already tested mtc_worker.
rozbb
left a comment
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.
Probably dumb question, but I'm not familiar with this pattern: what is a wshim and what is it intended to do? My guess: it is an object that knows how to serialize a log/registry/whatever to a particular endpoint that can be used for monitoring.
Another dumb question: why is this necessary when wrangler tail exists? Is this a common workaround to some known limitations in wrangler tail? Could it possibly be moved into a library eventually?
Finally, it'd be nice to have some comments explaining what's happening. This PR meaningfully increases complexity due to mutability and scoping tricks. It'd be good to make sure this is explained fully. And hopefully, written in a way that it can eventually be removed once we get a better logging story for wrangler.
|
67cd40b to
341565b
Compare
341565b to
e29901b
Compare
No description provided.