Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
inspector: added NodeRuntime domain #27600
inspector: added NodeRuntime domain #27600
Changes from all commits
d53eb62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I believe it needs to be communecated clearly on the purpose of this notification vs the
Runtime.executionContextDestroyed
- specifically, that the main context is retained and can still be used.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.
I updated 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.
It looks like this notification will only get sent to the sessions that requested it. Others will get nothing, not even an
Runtime.executionContextDestroyed
.I suggest sending other sessions
executionContextDestroyed
either as al else statement for this if or at a later point, once all sessions that requested a disconnect notification disconnect.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.
It is not possible to send
executionContextDestroyed
without dirty hacks. We need to intercept everyRuntime.enable
andRuntime.disable
calls to maintain runtime enabled state on node side. At the same time we can not just calls executionContextDestroyed on inspector since then inspector will mark context as destroyed and any later calls that uses Runtime will return an error.Based on how much hacks we need to add, how many clients there are in a wild right now, what is probability of using new client and old client at the same time I believe that we can just send nothing here. Protocol definition stated exactly this.
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.
I tried to adopt solution that you proposed and it seems like it works!