Add pre-set and post-set parameter callbacks

[minggangw]

Add addPreSetParametersCallback() / removePreSetParametersCallback() and addPostSetParametersCallback() / removePostSetParametersCallback() to the Node class. Pre-set callbacks run before validation and can transform the parameter list (e.g. coerce values); returning an empty list rejects the set. Post-set callbacks run after parameters are successfully applied, for side effects (e.g. reconfiguring a component). Both follow LIFO ordering (newest callback runs first), matching rclpy's add_pre_set_parameters_callback / add_post_set_parameters_callback behavior.

Modified: lib/node.js — Added _preSetParametersCallbacks and _postSetParametersCallbacks arrays. Modified _setParametersAtomically() to invoke pre callbacks before validation (merging all returned lists, rejecting on empty) and post callbacks after successful set and ParameterEvent publishing. Added 4 new public methods with JSDoc.

Modified: types/node.d.ts — Added PreSetParametersCallback and PostSetParametersCallback type aliases, and all 4 new methods to the Node interface.

New: test/test-pre-post-param-callbacks.js — 11 tests covering pre (value modification, empty-list rejection, LIFO ordering, removal), post (called on success, skipped on rejection, LIFO ordering, removal), and full chain (pre→on→post order, pre rejection stops chain, on rejection stops post).

Fix : #1469

[Copilot]

Pull request overview

This PR adds rclpy-style pre-set and post-set parameter callback hooks to Node, allowing parameter lists to be transformed before validation/setting and enabling side-effect handlers to run after a successful parameter update.

Changes:

• Add addPreSetParametersCallback / removePreSetParametersCallback and addPostSetParametersCallback / removePostSetParametersCallback APIs on Node.
• Invoke pre-set callbacks during _setParametersAtomically() before validation, and post-set callbacks after publishing the ParameterEvent.
• Add TypeScript typings and a new test suite covering ordering, rejection behavior, and removal.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File	Description
lib/node.js	Adds pre/post callback storage + public APIs; integrates callback invocation into _setParametersAtomically().
types/node.d.ts	Introduces PreSetParametersCallback / PostSetParametersCallback and adds the new Node methods.
test/test-pre-post-param-callbacks.js	New Mocha tests for pre/post callback behavior and ordering across the full set pipeline.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Current pre-set callback handling concatenates the returned arrays from each callback, and each callback is invoked with the original parameters list (not the output of the previous callback). This makes it impossible to chain transformations and can easily introduce duplicate entries for the same parameter name (leading to confusing validation / on-set callback inputs / ParameterEvent contents and “last write wins” behavior depending on array order). Consider applying pre-set callbacks as a pipeline: update parameters with each callback’s returned list before calling the next callback, and ensure the resulting list does not contain duplicate parameter names (or define/implement a deterministic conflict rule).

Suggested change

	let modified = [];
	for (const callback of this._preSetParametersCallbacks) {
	const result = callback(parameters);
	if (Array.isArray(result)) {
	modified.push(...result);
	}
	}
	parameters = modified;
	// Apply pre-set callbacks as a pipeline: each callback sees the
	// output of the previous one rather than the original list.
	let modified = Array.isArray(parameters) ? parameters : [];
	for (const callback of this._preSetParametersCallbacks) {
	const result = callback(modified);
	if (Array.isArray(result)) {
	// Replace the working list with the callback's result
	modified = result;
	}
	}
	// De-duplicate parameters by name in a deterministic way.
	// For named parameters, the last occurrence wins while preserving
	// overall order. Parameters without a string 'name' are preserved
	// in order after the named ones.
	const named = new Map();
	const unnamed = [];
	for (const param of modified) {
	if (param && typeof param.name === 'string') {
	named.set(param.name, param);
	} else {
	unnamed.push(param);
	}
	}
	parameters = [...named.values(), ...unnamed];

[Copilot]

The docs say “Returning an empty list rejects the set” / “If any pre-set callback returns an empty list, the set is rejected”, but the implementation only rejects when the combined parameters list ends up empty. With multiple pre callbacks, one callback can return [] and another can return non-empty and the set will still proceed, which contradicts the documented behavior. Recommend short-circuiting with a failure result immediately when any pre callback returns an empty array, and also treating non-array/undefined returns as an error (or as a rejection with a clear reason) rather than silently ignoring them.

[Copilot]

The “multiple pre-set callbacks” test currently only verifies call order, but it doesn’t assert how multiple callbacks’ returned parameter lists are combined/chained or how conflicts (same parameter name returned by multiple callbacks) are resolved. Adding assertions that validate the final parameter value (and that the final list has the expected contents) would prevent regressions in the pre-callback composition semantics.

[coveralls]

Changes unknown
when pulling 5e1f54c on minggangw:fix-1469
into ** on RobotWebTools:develop**.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (4)

lib/node.js:2023

• The PR description says pre-set callbacks are "merging all returned lists", but the implementation applies them as a pipeline (each callback receives and replaces the parameter list from the previous callback). Please update the PR description and/or public docs to reflect the pipeline behavior to avoid confusing API consumers.

  }

  /**
   * Repalce a list of declared parameters atomically.

lib/node.js:2180

• The new pre-set callback API is documented as running before validation, but the docs don’t state the execution order when multiple callbacks are registered. Since callbacks are added with unshift() and iterated in array order, they run LIFO (most recently added first). Please document that ordering explicitly (to match the behavior and tests).

   * @see [Node.addOnSetParametersCallback]{@link Node#addOnSetParametersCallback}
   * @see [Node.removeOnSetParametersCallback]{@link Node#removeOnSetParametersCallback}
   */

  /**
   * Add a callback to the front of the list of callbacks invoked for parameter declaration
   * and setting. No checks are made for duplicate callbacks.

lib/node.js:2168

• Docs for PreSetParametersCallback imply it only applies to setting, but the implementation runs pre-set callbacks for any path that calls _setParametersAtomically (including parameter declaration via declareParameter(s)). Please clarify in the callback and/or method docs that pre-set callbacks also run during declaration.

      reason: '',
    };
  }

  /**
   * This callback is called when declaring a parameter or setting a parameter.
   * The callback is provided a list of parameters and returns a SetParameterResult
   * to indicate approval or veto of the operation.

lib/node.js:2115

• Post-set callbacks run after parameters have been applied and the ParameterEvent published. If a post-set callback throws, _setParametersAtomically() will throw instead of returning {successful: true}, even though the parameters were already updated (and an event published), which can leave callers thinking the set failed. Consider wrapping each post callback invocation in a try/catch (e.g., log the error and continue) to preserve the method’s return contract and avoid inconsistent failure semantics.

          newParameters.push(parameter);
        } else {
          changedParameters.push(parameter);
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The post-set callback docs don’t specify the ordering when multiple callbacks are registered. The implementation adds callbacks with unshift() and iterates in order, which means LIFO (most recently added first). Please document that ordering explicitly so users can rely on it.

Suggested change

	* the set is rejected.
	* the set is rejected. Callbacks are invoked in last-in, first-out order: the
	* most recently added callback is invoked first.

[Copilot]

Docs for PostSetParametersCallback don’t mention that it will also be invoked during parameter declaration (since declarations use _setParametersAtomically). Please clarify whether post-set callbacks run after successful declaration as well, and document the behavior accordingly.