Missing trim() for options.name when options is an object

[jianyi-gronk]

I'm inquiring about the expected behavior of the options parameter in the tap method. specifically, whether the name should be allowed to be an empty string.

In the current _tap implementation, it only trims the name when a string is passed directly. For other cases (e.g., when an object is passed), should similar trimming be applied?

Current code:

_tap(type, options, fn) {
	if (typeof options === "string") {
		options = {
			name: options.trim()
		};
	} else if (typeof options !== "object" || options === null) {
		throw new Error("Invalid tap options");
	}
	if (typeof options.name !== "string" || options.name === "") {
		throw new Error("Missing name for tap");
	}
	if (typeof options.context !== "undefined") {
		deprecateContext();
	}
	options = Object.assign({ type, fn }, options);

	options = this._runRegisterInterceptors(options);
	this._insert(options);
}

My test results:

const hook = new SyncHook();

// Error: Missing name for tap
hook.tap(" ", () => {
  console.log("event-1");
});

// No error, tap is registered successfully
hook.tap({ name: " " }, () => {
  console.log("event-2");
});

hook.call();

Would it be better to modify it like this to ensure consistent trimming regardless of how the options are provided?
Suggested change:

_tap(type, options, fn) {
	if (typeof options === "string") {
		options = {
			name: options
		};
	} else if (typeof options !== "object" || options === null) {
		throw new Error("Invalid tap options");
	}
	if (typeof options.name === "string") {
		options.name = options.name.trim();
	}
	if (typeof options.name !== "string" || options.name === "") {
		throw new Error("Missing name for tap");
	}
	if (typeof options.context !== "undefined") {
		deprecateContext();
	}
	options = Object.assign({ type, fn }, options);

	options = this._runRegisterInterceptors(options);
	this._insert(options);
}

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jianyi-gronk / name: gronk (11d1987, ae415ff)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.96%. Comparing base (d3e85d0) to head (ae415ff).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #197   +/-   ##
=======================================
  Coverage   97.95%   97.96%           
=======================================
  Files          13       13           
  Lines         686      688    +2     
  Branches      110      111    +1     
=======================================
+ Hits          672      674    +2     
  Misses         13       13           
  Partials        1        1           

Flag	Coverage Δ
integration	97.96% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[alexander-akait]

@jianyi-gronk Can you fix lint? Thank you

[jianyi-gronk]

@alexander-akait done

[alexander-akait]

Thanks