pr feedback

Author: emily-shen

packages/wrangler/package.json

@@ -86,7 +86,6 @@
 		"selfsigned": "^2.0.1",
 		"source-map": "^0.6.1",
 		"unenv": "npm:[email protected]",
-		"which-pm-runs": "^1.1.0",
 		"workerd": "1.20241106.1",
 		"xxhash-wasm": "^1.0.1"
 	},

packages/wrangler/src/__tests__/metrics.test.ts

@@ -61,7 +61,7 @@ describe("metrics", () => {
 			vi.mocked(getOS).mockReturnValue("foo:bar");
 			vi.mocked(getWranglerVersion).mockReturnValue("1.2.3");
 			vi.mocked(getOSVersion).mockReturnValue("mock os version");
-			vi.mocked(getNodeVersion).mockReturnValue("1.1.1");
+			vi.mocked(getNodeVersion).mockReturnValue(1);
 			vi.mocked(getPlatform).mockReturnValue("mock platform");
 			vi.mocked(getConfigFileType).mockReturnValue("toml");
 			vi.useFakeTimers({
@@ -83,19 +83,7 @@ describe("metrics", () => {
 		describe("sendEvent()", () => {
 			it("should send a request to the default URL", async () => {
 				const requests = mockMetricRequest();
-				// {
-				// 	event: "some-event",
-				// 	properties: {
-				// 		category: "Workers",
-				// 		wranglerVersion: "1.2.3",
-				// 		os: "foo:bar",
-				// 		a: 1,
-				// 		b: 2,
-				// 	},
-				// },
-				// {
-				// 	"Sparrow-Source-Key": "MOCK_KEY",
-				// }
+
 				const dispatcher = await getMetricsDispatcher({
 					sendMetrics: true,
 				});
@@ -192,7 +180,7 @@ describe("metrics", () => {
 				wranglerVersion: "1.2.3",
 				osPlatform: "mock platform",
 				osVersion: "mock os version",
-				nodeVersion: "1.1.1",
+				nodeVersion: 1,
 				isFirstUsage: false,
 				configFileType: "toml",
 				isCI: false,
@@ -293,7 +281,7 @@ describe("metrics", () => {
 				msw.use(
 					http.post<Record<string, never>, { params: string | undefined }>(
 						`*/1/indexes/developers-cloudflare2/query`,
-						async ({}) => {
+						async () => {
 							vi.advanceTimersByTime(6000);
 							return HttpResponse.error();
 						},

packages/wrangler/src/config/index.ts

@@ -29,7 +29,7 @@ export type {
 	RawEnvironment,
 } from "./environment";
 
-function configFormat(
+export function configFormat(
 	configPath: string | undefined
 ): "jsonc" | "toml" | "none" {
 	if (configPath?.endsWith("toml")) {
@@ -91,9 +91,8 @@ export function readConfig(
 	requirePagesConfig?: boolean,
 	hideWarnings: boolean = false
 ): Config {
-	const { rawConfig, configPath: updateConfigPath } = parseConfig(
+	const { rawConfig, configPath: updateConfigPath } = readRawConfig(
 		configPath,
-		args,
 		requirePagesConfig
 	);
 
@@ -153,10 +152,8 @@ export function readConfig(
 	return config;
 }
 
-export const parseConfig = (
+export const readRawConfig = (
 	configPath: string | undefined,
-	// Include command specific args as well as the wrangler global flags
-	args: ReadConfigCommandArgs,
 	requirePagesConfig?: boolean
 ) => {
 	let rawConfig: RawConfig = {};

packages/wrangler/src/index.ts

@@ -12,7 +12,7 @@ import {
 	configFileName,
 	formatConfigSnippet,
 	loadDotEnv,
-	parseConfig,
+	readRawConfig,
 } from "./config";
 import { demandSingleValue } from "./core";
 import { CommandRegistry } from "./core/CommandRegistry";
@@ -1064,7 +1064,7 @@ export async function main(argv: string[]): Promise<void> {
 		// key to fetch) or flags
 
 		try {
-			const { rawConfig, configPath } = parseConfig(args.config, args, false);
+			const { rawConfig, configPath } = readRawConfig(args.config, false);
 			dispatcher = getMetricsDispatcher({
 				sendMetrics: rawConfig.send_metrics,
 				configPath,

packages/wrangler/src/metrics/helpers.ts

@@ -1,13 +1,18 @@
 import os from "node:os";
-import whichPmRuns from "which-pm-runs";
 import { version as wranglerVersion } from "../../package.json";
+import { configFormat } from "../config";
+import {
+	getPackageManager as _getPackageManager,
+	getPackageManagerName,
+} from "../package-manager";
 
 export function getWranglerVersion() {
 	return wranglerVersion;
 }
 
-export function getPackageManager() {
-	return whichPmRuns()?.name;
+export async function getPackageManager() {
+	const packageManager = await _getPackageManager(process.cwd());
+	return getPackageManagerName(packageManager);
 }
 
 // used by "new" metrics
@@ -36,22 +41,10 @@ export function getOSVersion() {
 }
 
 export function getNodeVersion() {
-	return process.version;
+	const nodeVersion = process.versions.node;
+	return parseInt(nodeVersion.split(".")[0]);
 }
 
 export function getConfigFileType(configPath: string | undefined) {
-	if (configPath === undefined) {
-		return "none";
-	}
-	if (configPath.endsWith(".toml")) {
-		return "toml";
-	}
-	if (configPath.endsWith(".json")) {
-		return "json";
-	}
-	if (configPath.endsWith(".jsonc")) {
-		return "jsonc";
-	}
-	/** shouldn't ever get here */
-	return "invalid";
+	return configFormat(configPath);
 }

packages/wrangler/src/metrics/metrics-dispatcher.ts

@@ -1,6 +1,6 @@
 import chalk from "chalk";
 import { fetch } from "undici";
-import _isInteractive from "../is-interactive";
+import isInteractive from "../is-interactive";
 import { logger } from "../logger";
 import { CI } from "./../is-ci";
 import {
@@ -28,16 +28,9 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) {
 	const SPARROW_SOURCE_KEY = process.env.SPARROW_SOURCE_KEY ?? "";
 	const requests: Array<Promise<void>> = [];
 	const wranglerVersion = getWranglerVersion();
-	const osPlatform = getPlatform();
-	const osVersion = getOSVersion();
-	const nodeVersion = getNodeVersion();
-	const packageManager = getPackageManager();
-	const isFirstUsage = readMetricsConfig().permission === undefined;
-	const isCI = CI.isCI();
-	const isInteractive = _isInteractive();
 	const amplitude_session_id = Date.now();
-	const configFileType = getConfigFileType(options.configPath);
 	let amplitude_event_id = 0;
+
 	/** We redact strings in arg values, unless they are named here */
 	const allowList = {
 		// applies to all commands
@@ -95,20 +88,20 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) {
 				amplitude_session_id,
 				amplitude_event_id: amplitude_event_id++,
 				wranglerVersion,
-				osPlatform,
-				osVersion,
-				nodeVersion,
-				packageManager,
-				isFirstUsage,
-				configFileType,
-				isCI,
-				isInteractive,
+				osPlatform: getPlatform(),
+				osVersion: getOSVersion(),
+				nodeVersion: getNodeVersion(),
+				packageManager: await getPackageManager(),
+				isFirstUsage: readMetricsConfig().permission === undefined,
+				configFileType: getConfigFileType(options.configPath),
+				isCI: CI.isCI(),
+				isInteractive: isInteractive(),
 				argsUsed,
 				argsCombination,
 			};
-			// we redact all args unless they are in the allowList
-			const allowedKeys = getAllowedKeys(allowList, properties.command ?? "");
-			properties.args = redactArgValues(properties.args ?? {}, allowedKeys);
+			// get the args where we don't want to redact their values
+			const allowedArgs = getAllowedArgs(allowList, properties.command ?? "");
+			properties.args = redactArgValues(properties.args ?? {}, allowedArgs);
 			await dispatch({
 				name,
 				properties: {
@@ -154,8 +147,8 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) {
 
 		logger.debug(`Metrics dispatcher: Posting data ${JSON.stringify(body)}`);
 
-		// Do not await this fetch call.
-		// Just fire-and-forget, otherwise we might slow down the rest of Wrangler.
+		// Don't await fetch but make sure requests are resolved (with a timeout)
+		// before exiting Wrangler
 		const request = fetch(`${SPARROW_URL}/api/v1/event`, {
 			method: "POST",
 			headers: {
@@ -235,7 +228,7 @@ const sanitiseUserInput = (argsWithValues: Record<string, unknown>) => {
 	return result;
 };
 
-const getAllowedKeys = (
+const getAllowedArgs = (
 	allowList: Record<string, string[]> & { "*": string[] },
 	key: string
 ) => {

packages/wrangler/src/metrics/types.ts

@@ -14,9 +14,9 @@ export type CommonEventProperties = {
 	 */
 	packageManager: string | undefined;
 	/**
-	 * The version of node that the Wrangler client is running on.
+	 * The major version of node that the Wrangler client is running on.
 	 */
-	nodeVersion: string;
+	nodeVersion: number;
 	/**
 	 * Whether this is the first time the user has used the wrangler client.
 	 */