feat: Enable log redirection and update docs #522
Conversation
github-actions
bot
added
t-tooling
Pijukatel
force-pushed
the
expose-log-redirection-and-add-docs
branch
from
4fa4535 to
a01f980
Compare
Pijukatel
force-pushed
the
expose-log-redirection-and-add-docs
branch
from
a01f980 to
1cffc5e
Compare
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
Thank you @Pijukatel! I have just a few points to discuss:
docs/guides/log_redirection.mdx
Outdated
| await Actor.exit(); | ||
| ``` | ||
| Each default redirect log entry will have a specific format. After the timestamp, it will contain cyan colored text that will contain the redirect information - the other Actor's name and the run ID. The rest of the log message will be printed in the same manner as the parent Actor's log is configured. |
There was a problem hiding this comment.
After the timestamp
The timestamp is only added by the Platform, so locally, this message will start with the colored redirect information, right?
But even if so, I don't really see a reason to change this, this is IMO too minuscule to confuse anybody too much.
| actorId: string, | ||
| input?: unknown, | ||
| options: CallOptions = {}, | ||
| options: StartOptions = {}, |
There was a problem hiding this comment.
This is curious, nice catch 👀
I see that StartOptions is not larger than CallOptions, so this change might cause some type errors though. If the user was e.g. using the waitSecs parameter with start method - even though it would be likely ignored, it might start failing (on type level) now.
Idk, this is probably a fix (so not a breaking change), but IMO still something to consider
There was a problem hiding this comment.
Yes, I edited PR description to explicitly mention this change. But would you prefer to do this as separate PR with fix or is it ok to be included in this one?
There was a problem hiding this comment.
Right, that's a good point - we probably should do it in a separate PR, so it shows in the changelog (and is easier to revert if needed).
Co-authored-by: Jindřich Bär <[email protected]>
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
lgtm, thank you for the quick fixes 🚀
please address the type change in a different PR and feel free to merge 👍
apify-clientdependency to enable log redirection.