fix(projectify): optimize replaceProjectIdToken performance

[surbhigarg92]

This PR optimizes the core replaceProjectIdToken utility function for maximum performance, memory efficiency, and safety.

By moving to an optimized Selective-Write In-Place Mutation design and restructuring control flow, we eliminate exponential recursive redundancy on arrays, bypass unnecessary checks on primitive values, and prevent any write operations on unmodified object/array elements.

This keeps memory allocations at absolute zero (avoiding GC ticks) and allows frozen arrays and objects to be traversed safely without throwing TypeErrors as long as they do not contain placeholders to replace.

[gemini-code-assist]

Code Review

This pull request refactors the replaceProjectIdToken utility to optimize performance and adds tests for handling values without placeholders. The review feedback highlights two issues with the new implementation: mutating input arrays in-place (which is a regression from the original .map() implementation and can cause side effects or crash on frozen arrays) and performing unnecessary write operations on object properties. The reviewer suggested using a copy-on-write strategy for arrays and checking for actual changes before writing to object properties.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the replaceProjectIdToken function to optimize performance by avoiding unnecessary allocations when no placeholders are present. It also adds tests for frozen arrays and objects. However, mutating the input array in-place is a breaking change compared to the original implementation (which used .map()) and will throw a TypeError if a frozen array contains a placeholder. A copy-on-write approach is suggested to safely preserve backward compatibility and prevent runtime errors.

[gemini-code-assist]

Mutating the input array in-place is a breaking change compared to the original implementation, which used .map() to return a new array. If a caller passes an array and expects it to remain unmodified, this in-place mutation will introduce unexpected side effects. Furthermore, if a frozen array contains a placeholder, attempting to mutate it in-place will throw a TypeError at runtime.

To preserve the performance benefits of avoiding allocations when no placeholders are present, while maintaining safety and backward compatibility, we can use a Copy-on-Write approach. We only clone the array if we actually detect a modified element.

Suggested change

	if (Array.isArray(value)) {
	for (let i = 0; i < value.length; i++) {
	const original = value[i];
	const processed = replaceProjectIdToken(original, projectId);
	if (processed !== original) {
	value[i] = processed;
	}
	}
	return value;
	}
	if (Array.isArray(value)) {
	let cloned: any[] | null = null;
	for (let i = 0; i < value.length; i++) {
	const original = value[i];
	const processed = replaceProjectIdToken(original, projectId);
	if (processed !== original) {
	if (!cloned) {
	cloned = [...value];
	}
	cloned[i] = processed;
	}
	}
	return cloned || value;
	}

[surbhigarg92]

The original implementation was already mutating values in-place for all nested objects

// Original implementation mutated objects in-place:
for (const opt in value) {
  if (value.hasOwnProperty(opt)) {
    value[opt] = replaceProjectIdToken(value[opt], projectId);
  }
}

To address the concern about frozen arrays/objects without triggering new allocations, we implemented a Selective-Write strategy.

const original = value[i];
const processed = replaceProjectIdToken(original, projectId);
if (processed !== original) {
  value[i] = processed; // Only writes if a placeholder was actually found & changed!
}

[pearigee]

There are a lot of assertions in both of these added test cases. Could you break this into multiple more focused tests?

For example:

it('should not modify primitives, () => {
    assert.strictEqual(replaceProjectIdToken(123, PROJECT_ID), 123);
    assert.strictEqual(replaceProjectIdToken(true, PROJECT_ID), true);
    assert.strictEqual(replaceProjectIdToken(null, PROJECT_ID), null);
    assert.strictEqual(replaceProjectIdToken(undefined, PROJECT_ID), undefined);
});

it('should not modify arrays without the placeholder, () => {
   const array = [1, 2, 3];
   assert.strictEqual(replaceProjectIdToken(array, PROJECT_ID), array);
});

// etc . . .

[pearigee]

I think this could be simplified using more modern syntax:

for (const key of Object.keys(value)) {
    value[key] = replaceProjectIdToken(value[key], projectId);
}

I don't think we need the additional if (processed !== original) because this function has already been modified to return the references in place?

[pearigee]

Should we combine this check for Buffer and Stream with the other checks above for null and non-object primitives?

// Early return for null, primitives (i.e. booleans, numbers), Buffers, and Streams. These are non-traversable leaf nodes.
// Note this must come after the string check.
if (
    value === null ||
    typeof value !== 'object' ||
    value instanceof Buffer ||
    value instanceof Stream
) {
    return value;
}