Skip to content

Conversation

@Ayoub-Mabrouk
Copy link
Contributor

@Ayoub-Mabrouk Ayoub-Mabrouk commented Dec 30, 2025

The issue affects:

  • Runtime code: src/log.ts uses .replaceAll() in formatCmd() function
  • Build scripts: Multiple build scripts use .replaceAll() which prevents building on Node 12-14

Solution

This PR replaces all .replaceAll() calls with .replace(/pattern/g, ...) to maintain backward compatibility with Node 12.17.0+ as specified in the engines field.

Changes Made

  • src/log.ts: Replace .replaceAll('\n', ...) with .replace(/\n/g, ...)
  • scripts/build-versions.mjs: Replace 3 .replaceAll() calls with regex-based .replace()
  • scripts/build-js.mjs: Replace 6 .replaceAll() calls with regex-based .replace()
  • scripts/build-dts.mjs: Replace .replaceAll() with regex-based .replace()
  • Build artifacts: Regenerated build/core.cjs and build/3rd-party-licenses

I chose to fix the code rather than update the engines field to >= 15.0.0 for the following reasons:

  1. Backward Compatibility: The project explicitly claims support for Node 12.17.0+ in package.json. Changing this would be a breaking change for users on Node 12-14.
  2. Minimal Impact: The fix is straightforward - replacing .replaceAll() with .replace(/pattern/g, ...) maintains identical functionality while supporting older Node versions.
  3. No Functional Changes: The regex-based approach produces the same results as .replaceAll(), just with broader compatibility.

Alternative Approach

If the maintainers prefer to drop support for Node 12-14, I'm happy to create a separate PR that:

  • Updates engines field to ">= 15.0.0"
  • Keeps .replaceAll() as-is
  • Updates documentation accordingly

This would be a cleaner solution if backward compatibility with Node 12-14 is no longer a priority.

@antongolub
Copy link
Collaborator

Hey, @Ayoub-Mabrouk,

Lets apply the fix only to the src/log.ts.

@Ayoub-Mabrouk Ayoub-Mabrouk force-pushed the fix/replaceall-node12-compatibility-clean branch from cf70678 to 1909817 Compare January 27, 2026 23:01
@Ayoub-Mabrouk
Copy link
Contributor Author

Hey, @Ayoub-Mabrouk,

Lets apply the fix only to the src/log.ts.

Hello @antongolub, thanks for the feedback, done.
hope you check this pr when you have time.

@antongolub antongolub changed the title fix(log): fix Node 12+ compatibility by replacing replaceAll fix(log): fix nodejs12 compatibility Feb 1, 2026
@antongolub
Copy link
Collaborator

@Ayoub-Mabrouk ,

Commits must have verified signatures

Ayoub-Mabrouk and others added 3 commits February 2, 2026 01:14
.replaceAll() requires Node 15+ but engines field claims >= 12.17.0.
Use .replace(/\n/g, ...) instead to maintain backward compatibility.
* chore: apply prettier
* chore: update jsr to v0.14.2
@Ayoub-Mabrouk Ayoub-Mabrouk force-pushed the fix/replaceall-node12-compatibility-clean branch from 95c39ab to 428870b Compare February 2, 2026 00:14
@antongolub antongolub merged commit 446b755 into google:main Feb 2, 2026
30 checks passed
@antongolub antongolub added the bug label Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants