vm: Removed Watchdog dependency on Environment.#3274
vm: Removed Watchdog dependency on Environment.#3274idoby wants to merge 1 commit intonodejs:masterfrom
Conversation
Removed the Watchdog class' dependency on Environment to allow it to be used by addons.
|
Changes LGTM but node_watchdog.h should not be considered part of the official C++ API / ABI, that's restricted to node.h, node_buffer.h and node_object_wrap.h. |
|
I understand. Is there a better way to access similar functionality from an external module? I have a Contextify-like module and I'm trying to implement execution timeouts for it, but would like to avoid replicating all the Watchdog code. |
|
Not really, I think. I'd probably just copy over the code. |
mscdex
added
the
c++
|
@bnoordhuis Should this be closed then? |
Remove the Watchdog class' dependency on Environment. No functional changes, only code cleanup. PR-URL: #3274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
The changes themselves LGTM and they make it a little more loosely coupled. I landed it in b244f13, thanks Ido. |
Remove the Watchdog class' dependency on Environment. No functional changes, only code cleanup. PR-URL: #3274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Should this be included in LTS? |
|
I don't think so. |
|
cool just getting a feel for it. So this would likely be considered a change in functionality |
|
@thealphanerd no not really, it's not really a fix though. More like refactoring, I'm not really sure this qualifies as such. |
|
Yeah, agree with @Fishrock123 |
|
I think technically this would qualify as an ABI change, no? We don't ship node_watchdog.h with headers so it's unlikely to be actually used but perhaps this should have been semver-major anyway? Labelling with |
7 participants
Removed the Watchdog class' dependency on Environment to allow it
to be used by addons.