This proposal is to refactor the rclnodejs design to properly support rcl contexts and to better use context in certain api's. My rationale is that the current rclnodejs design effectively restricts users to a single context. Yet at the rcl level, multiple contexts are supported. This is not a big deal in most cases but unnecessarily restricts more advance uses of rcl context such as the rclnodejs implementation of the Rate class which is hacky workaround. And the current visibility of context in a number of api can be refined to only a limited set of locations making for cleaner api.
Benefit
- cleaner user api
- conceptually maps to rcl closely
- can create and use multiple contexts, such as how we implemented Rate in it's own context
Background
A rclnodejs Context is a JS interface to the underlying rcl context. I like to think of the rcl context as a container for rcl objects such as nodes, publishers, actions, etc... The rcl design is such that it can host multiple contexts within a process which can lead to some clever advanced uses. The design of rclnodejs restricts us to a single instance of context. Specifically, the rclnodejs singleton maintains a global list of all nodes, separate from a node's context. When rclnodejs.shutdown(context) is called, all nodes regardless of their context (container) are stopped which is not typically desired. Additionally context is passed as an argument to several node methods for which the node's implicit context reference should be used.
Recommendations:
0. Overall goal is to limit the visibility and use of context to only the absolute essential places of dependency. And to do this with minimal changes and disruption such that all but most advanced uses of the current api are impacted.
-
Revise rclnodejs to properly manage the relationship between each context and nodes created in that context.
-
Remove the context argument from Node.createTimer() and Node.createGuard()` and use the node's implicit context reference (i.e., instance var).
-
Clean up the hacked Rate implementation to use it's own context without resorting to use of private api.
Concerns
A main concern is breaking api changes. I suspect that removing the context parameters from api will not impact many users because most user of the api have relied on the default value of the context parameter. We could start with a deprecation warning where applicable that context parameter is not longer used and will be removed in a future update.
Thoughts?
This proposal is to refactor the rclnodejs design to properly support rcl contexts and to better use context in certain api's. My rationale is that the current rclnodejs design effectively restricts users to a single context. Yet at the rcl level, multiple contexts are supported. This is not a big deal in most cases but unnecessarily restricts more advance uses of rcl context such as the rclnodejs implementation of the Rate class which is hacky workaround. And the current visibility of context in a number of api can be refined to only a limited set of locations making for cleaner api.
Benefit
Background
A rclnodejs Context is a JS interface to the underlying rcl context. I like to think of the rcl context as a container for rcl objects such as nodes, publishers, actions, etc... The rcl design is such that it can host multiple contexts within a process which can lead to some clever advanced uses. The design of rclnodejs restricts us to a single instance of context. Specifically, the rclnodejs singleton maintains a global list of all nodes, separate from a node's context. When rclnodejs.shutdown(context) is called, all nodes regardless of their context (container) are stopped which is not typically desired. Additionally context is passed as an argument to several node methods for which the node's implicit context reference should be used.
Recommendations:
0. Overall goal is to limit the visibility and use of context to only the absolute essential places of dependency. And to do this with minimal changes and disruption such that all but most advanced uses of the current api are impacted.
Revise rclnodejs to properly manage the relationship between each context and nodes created in that context.
Remove the context argument from
Node.createTimer() andNode.createGuard()` and use the node's implicit context reference (i.e., instance var).Clean up the hacked Rate implementation to use it's own context without resorting to use of private api.
Concerns
A main concern is breaking api changes. I suspect that removing the context parameters from api will not impact many users because most user of the api have relied on the default value of the context parameter. We could start with a deprecation warning where applicable that context parameter is not longer used and will be removed in a future update.
Thoughts?