-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 RuntimeState and keylogger to it #2487
Conversation
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.
👍 Something like this LGTM. I think you're not covering the case testTypeArchive
branch in initializeFirstRunner()
, so some further refactoring might be needed, but I like the general approach
Yeah, I also will remove the functions which don't take teh state, but didn't want to do any more changes than needed to actually get a PoC |
5f8266b
to
46babf0
Compare
Moving tlsconfig out of js.Runner seemed like way too big of a yak shave to start on it - given the amount of refactoring I already needed to do to make this possible at all -so for now i just skip tls checking. |
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.
LGTM besides #2487 (review)
I also made this small unrelated change b710000 ... kind of wonder if I should make the |
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.
LGTM! 🎉 ❤️
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.
Tested and approved on Windows! Great job, and super useful indeed! 🪟 🎉
Good stuff! Thanks @mstoykov |
Based on #2485 (comment)
fixes #1043
Changelog entry (under new features):