Added types for url-utils #685

vershwal · 2025-11-12T21:55:51Z

ref https://linear.app/ghost/issue/PRO-1535/

This PR adds types to the url-util files and removes the //@ts-nocheck comment

Note

Converts url-utils to TypeScript with typed optioned APIs and default exports, updates internals for CDN handling/early exits, and adjusts tests and package config.

TypeScript Migration:
- Convert UrlUtils and all utilities to TypeScript; remove // @ts-nocheck and add comprehensive typings via utils/types.ts.
- Add src/index.ts and export UrlUtils as default.
Module/Build Config:
- Switch from CommonJS requires to ES imports/exports with default exports across utils.
- Update package.json to main: lib/index.js, types: lib/index.d.ts; adjust lint paths.
Functional/Internal Updates:
- Add early-exit URL match helper (build-early-exit-match) and extend transform functions to consider CDN base URLs for images/files/media.
- Normalize option handling and itemPath overloading across helpers; minor behavior clarifications (e.g., protocol normalization in relativeToTransformReady, unknown permalink segments yield undefined).
Tests:
- Update imports to use .default from compiled lib and adapt rewire targets accordingly.

^{Written by Cursor Bugbot for commit 201280b. This will update automatically on new commits. Configure here.}

cursor · 2025-11-12T21:55:55Z

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 3.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

- This adds types to simple utilities with no dependencies on other utils

- Add types to all common utility files - Update require() calls in format-specific files to use .default (temporary until those files are converted to ES6 imports) - Updated tests

- Added types to all HTML format utilities - Updated markdown files and index.ts to use ES6 imports for HTML files - Updated tests

- Added types to all Lexical format utilities - Updated markdown files and index.ts to use ES6 imports for Lexical files - Updated tests

- Added types to all Markdown format utilities - Updated index.ts to use ES6 imports for Markdown files - Updated tests

- Added types to all Mobiledoc format utilities - Updated index.ts to use ES6 imports for Lexical files - Updated tests

- Added types to all plaintext format utilities - Updated index.ts to use ES6 imports for plaintext files - Updated tests

- Added Types to Main Class - Updated tests

- Moved index.ts from root to src/index.ts to align with TypeScript structure - Updated package.json

vershwal · 2025-11-20T08:27:42Z

bugbot run

vershwal · 2025-11-20T08:27:59Z

@cursor review

Copilot

Pull Request Overview

This PR adds TypeScript types to the url-utils package by converting JavaScript files to TypeScript and removing @ts-nocheck comments. The main changes include creating type definitions, converting from CommonJS (module.exports/require) to ES modules (export/import), and updating test files to use .default imports for the newly converted modules.

Key changes:

Created comprehensive type definitions in a new types.ts file
Converted all utility functions from CommonJS to ES modules with TypeScript types
Updated the main UrlUtils class with proper TypeScript types and access modifiers
Modified test files to access default exports correctly

Reviewed Changes

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

Show a summary per file

File	Description
`packages/url-utils/src/utils/types.ts`	New file containing comprehensive type definitions for all utility functions
`packages/url-utils/src/UrlUtils.ts`	Main class converted to TypeScript with proper types and class members
`packages/url-utils/src/index.ts`	New entry point using ES module exports
`packages/url-utils/src/utils/*.ts`	All utility files converted from JS to TS with type annotations
`packages/url-utils/test/unit/*/.test.js`	Test files updated to import `.default` from converted modules
`packages/url-utils/package.json`	Updated entry points for the TypeScript build
`packages/url-utils/index.js`	Deleted in favor of compiled output

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

Copilot · 2025-11-20T08:40:58Z

packages/url-utils/src/utils/replace-permalink.ts

+        // Unknown route segment - return 'undefined' string
+        return 'undefined';


Returning the string 'undefined' for unknown route segments changes behavior from the original code which returned the matched string unchanged. This could break existing permalinks that use custom/unknown route segments. The original behavior should be preserved by returning match instead of 'undefined'.

The original JavaScript code didn't return a value for unknown route segments, which JavaScript's String.replace() automatically converts to the string "undefined". So this does not change the functionality. This behavior is explicitly tested here.

Copilot · 2025-11-20T08:40:58Z

packages/url-utils/src/UrlUtils.ts

+        // Original code passes options as third parameter (itemPath), preserving that behavior
+        return utils.relativeToAbsolute(url, this.getSiteUrl(), options || null, undefined);


This comment and implementation are incorrect. The original code relativeToAbsolute(url, this.getSiteUrl(), options) passes options as the 4th parameter (_options), not as the 3rd parameter (itemPath). The call should be utils.relativeToAbsolute(url, this.getSiteUrl(), null, options) to properly pass options while leaving itemPath as null.

Suggested change

// Original code passes options as third parameter (itemPath), preserving that behavior

return utils.relativeToAbsolute(url, this.getSiteUrl(), options || null, undefined);

// Pass null for itemPath and options as the fourth parameter, per correct parameter order

return utils.relativeToAbsolute(url, this.getSiteUrl(), null, options);

The relativeToAbsolute function's 4th parameter (_options) is optional, so passing undefined as the 4th parameter doesn't change functionality. Both of these produce the same result:

relativeToAbsolute(url, rootUrl, options) - 3 parameters

relativeToAbsolute(url, rootUrl, options, undefined) - 4 parameters

The function's logic checks !_options (line 37), which evaluates to true when _options is undefined, whether it's not provided or explicitly passed. The current implementation preserves the original 3-parameter call behavior.

I can remove this comment though, this is misleading.

Copilot · 2025-11-20T08:40:59Z

packages/url-utils/src/utils/url-join.ts

+    if (!options.rootUrl) {
+        return url;
+    }


Adding an early return when options.rootUrl is empty changes the function behavior. The original code would call deduplicateSubdirectory(url, '') even with an empty rootUrl. This could break existing functionality that relies on the subdirectory deduplication logic. Remove this early return to preserve original behavior.

The original code always calls deduplicateSubdirectory(url, options.rootUrl) regardless of whether rootUrl is empty.

However, I've kept the early return for the following reasons:

Functionally equivalent: If deduplicateSubdirectory were called with an empty rootUrl, it would:

Convert '' to '/'

Parse it (pathname would be /)

Hit the check if (parsedRoot.pathname === '/') on line 19

Return the url unchanged

This is exactly what the early return does - returns the url unchanged.

Better error handling: The original code crashes with "Invalid URL" error when rootUrl is empty, while the early return avoids the crash and produces the same result.

No practical difference: In practice, urlJoin is never called with empty/undefined rootUrl in the codebase (all usages pass a valid rootUrl), so this only affects edge cases. But when it does, the early return provides a better experience without changing the functional output.

allouis

First pass - I've left some comments/questions but overall this is feeling good.

One thing we need to think about is that some packages (incorrectly) read from the lib path - so I think we're going to need to make sure that consumers can somehow read the individual helpers - here's an example of what I mean https://github.com/TryGhost/Koenig/blob/0bac7fb5c62dcf037376a956d16bc47492d8fb97/packages/kg-default-cards/lib/cards/file.js#L5

allouis · 2025-12-03T07:04:22Z

packages/url-utils/src/utils/replace-permalink.ts

        },
        author: function () {
-            return resource.primary_author.slug;
+            return resource.primary_author?.slug ?? primaryTagFallback;


Does this change functionality by adding a fallback?

I think maybe 'undefined' matches the currently functionality

You're right, this changes behavior from the original code, which would throw an error when primary_author is missing. I've updated it to return 'undefined'.

allouis · 2025-12-03T07:05:57Z

packages/url-utils/src/utils/replace-permalink.ts

        },
        id: function () {
-            return resource.id;
+            return String(resource.id);


Why do we need to use String here?

I added it because the tests pass id as a number (e.g., id: 3 in the tests), but since the interface defines id: string and the original code returned resource.id directly, I'm removing it to match the original implementation.

allouis · 2025-12-03T07:21:24Z

packages/url-utils/src/utils/absolute-to-transform-ready.ts

    // convert to relative with stripped subdir
    // always returns root-relative starting with forward slash
-    const rootRelativeUrl = absoluteToRelative(url, root, options);
+    const rootRelativeUrl = absoluteToRelative(url, root, options as AbsoluteToRelativeOptionsInput);


Why do we need to type cast here? Can we instead make sure that options is of type AbsoluteToTransformReadyOptions and then it overlaps?

Good catch! I've updated AbsoluteToTransformReadyOptions to extend AbsoluteToRelativeOptions, which makes the types properly overlap. Removed type casts.

allouis · 2025-12-03T07:25:25Z

packages/url-utils/src/utils/build-early-exit-match.ts

    const candidates = [siteUrl, options.imageBaseUrl, options.filesBaseUrl, options.mediaBaseUrl]
-        .filter(Boolean)
-        .map((value) => {
+        .filter((value): value is string => Boolean(value))


value is string doesn't match the actual implementation 🤔

The Boolean(value) check is making sure it's a non empty string - I think we should change this line to the following which is slightly more correct and at least enforces the string type

.filter((value: string | null): value is string => typeof value === 'string' && value.length > 0)

allouis · 2025-12-03T07:28:33Z

packages/url-utils/src/utils/build-early-exit-match.ts

            return normalized.replace(/\/$/, '');
        })
-        .filter(Boolean)
+        .filter((value): value is string => Boolean(value))


I think in this case we can just do (value: string): boolean => Boolean(value)

allouis · 2025-12-08T04:23:33Z

packages/url-utils/src/utils/relative-to-absolute.ts

    // return the path as-is if it's not an asset path and we're only modifying assets
    if (options.assetsOnly) {
-        const staticImageUrlPrefixRegex = new RegExp(options.staticImageUrlPrefix);
+        // staticImageUrlPrefix is always set from defaultOptions via Object.assign


I don't think this is true 🤔

const defaults = {prop: 'val'} Object.assign({}, defaults, {other: 'thing'}) // has default Object.assign({}, defaults, {other: 'thing', prop: 'value'}) // has passed value // but if we explicity pass undefined or null Object.assign({}, defaults, {other: 'thing', prop: null}) // has null Object.assign({}, defaults, {other: 'thing', prop: undefined}) // has undefined

There's likely a bug here - but it's an existing one and i'm not sure how much of an impact it has - only if we're passing assetsOnly and the url has the string 'undefined' inside of it

Either way though - we should not use the ! operator here IMO

Okay, ya 🤔
Using as string type assertion to preserve the original behavior, including the existing bug where undefined/null would be coerced.

allouis · 2025-12-08T04:40:32Z

packages/url-utils/src/utils/markdown-absolute-to-relative.ts


    options.earlyExitMatchStr = options.ignoreProtocol ? siteUrl.replace(/http:|https:/, '') : siteUrl;
-    options.earlyExitMatchStr = options.earlyExitMatchStr.replace(/\/$/, '');
+    if (options.earlyExitMatchStr) {


Not 100% sure why we need this check 🤔

hmm..ya I don't think it's needed. Can't remember, maybe just added it defensively during the migration. Removed it for now.

- replace-permalink: Return 'undefined' for missing author (match original behavior) - replace-permalink: Remove unnecessary String() conversion for id - absolute-to-transform-ready: Extend AbsoluteToRelativeOptions to remove type casts - build-early-exit-match: Fix type guards to properly check string type and length - markdown-absolute-to-relative: Remove unnecessary earlyExitMatchStr check - relative-to-absolute: Remove incorrect comment and unsafe ! operator, use type assertion

cursor

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on January 3

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

cursor · 2025-12-08T11:15:20Z

packages/url-utils/src/utils/plaintext-absolute-to-transform-ready.ts

-    return plaintext.replace(linkRegex, function (fullMatch, url) {
-        const newUrl = absoluteToTransformReady(`${url}`, rootUrl, options);
+    return plaintext.replace(linkRegex as RegExp, function (fullMatch: string, url: string): string {
+        const newUrl = absoluteToTransformReady(`${url}`, rootUrl, finalOptions);


Bug: Null regex cast causes runtime error in plaintext transform

The function buildLinkRegex can return null when rootUrl is falsy and no CDN options are provided, but at line 46 the result is force-cast to RegExp via as RegExp and passed to String.prototype.replace(). This will cause a runtime error when linkRegex is null. The code lacks a null check before calling replace() and should either return the plaintext unchanged when linkRegex is null, or ensure the regex is always valid.

I don't think this cause a runtime error. The original JavaScript code also called replace(linkRegex, ...) without a null check, and JavaScript's replace(null, ...) simply returns the original string unchanged. This is the intended behavior - if there are no base URLs to match, the plaintext is returned unchanged. The as RegExp type cast is only for TypeScript compatibility and doesn't affect runtime behavior.

packages/url-utils/src/utils/relative-to-transform-ready.ts

packages/url-utils/src/UrlUtils.ts

cursor · 2025-12-08T11:46:35Z

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

packages/url-utils/src/utils/relative-to-transform-ready.ts

- Use assignOptions instead of Object.assign in UrlUtils constructor to preserve undefined handling (filters out undefined values from user options, preserving defaults) - Restore overrideOptions pattern in relativeToTransformReady to ensure secure: false is always forced regardless of user input - Add trailing slash after replacement string in relativeToTransformReady to match original JavaScript behavior

cursor · 2025-12-08T12:23:23Z

packages/url-utils/src/utils/replace-permalink.ts

        },
        author: function () {
-            return resource.primary_author.slug;
+            return resource.primary_author?.slug ?? 'undefined';


Bug: Inconsistent fallback produces literal 'undefined' in URLs

The author function returns the literal string 'undefined' when primary_author is missing, while the primary_author function correctly falls back to primaryTagFallback ('all'). This means a permalink pattern using :author would produce URLs like /undefined/my-post/ when the author is missing, instead of using a sensible fallback. The author function is missing the same fallback logic that primary_author uses.

We have discussed this above. This is expected.

vershwal force-pushed the urlUtilsTSMigrationSetup branch 2 times, most recently from 34cd4e4 to 41554dc Compare November 13, 2025 07:59

vershwal marked this pull request as draft November 13, 2025 09:14

vershwal force-pushed the urlUtilsTSMigrationSetup branch from 108f965 to 9bb7431 Compare November 13, 2025 12:06

vershwal force-pushed the typesForUrlUtils branch from c8843d5 to ab5c79c Compare November 17, 2025 05:25

vershwal force-pushed the urlUtilsTSMigrationSetup branch 3 times, most recently from 3d96b0d to 8045015 Compare November 19, 2025 12:28

Base automatically changed from urlUtilsTSMigrationSetup to main November 19, 2025 12:33

vershwal added 10 commits November 19, 2025 23:15

Added Types to Simple Utility Functions

dd62bac

- This adds types to simple utilities with no dependencies on other utils

Add TypeScript types to transform utility files

7b4e571

- Add types to all common utility files - Update require() calls in format-specific files to use .default (temporary until those files are converted to ES6 imports) - Updated tests

Added TypeScript types to HTML format files

5795a7a

- Added types to all HTML format utilities - Updated markdown files and index.ts to use ES6 imports for HTML files - Updated tests

Updated Lexical format files

fcc7436

- Added types to all Lexical format utilities - Updated markdown files and index.ts to use ES6 imports for Lexical files - Updated tests

Updated Markdown format files

6361779

- Added types to all Markdown format utilities - Updated index.ts to use ES6 imports for Markdown files - Updated tests

Updated Mobiledoc format files

18d88a9

- Added types to all Mobiledoc format utilities - Updated index.ts to use ES6 imports for Lexical files - Updated tests

Updated plaintext format files

615872c

- Added types to all plaintext format utilities - Updated index.ts to use ES6 imports for plaintext files - Updated tests

Updated Index File

7549bac

Updated UrlUtils.ts

9d3d43c

- Added Types to Main Class - Updated tests

Updated index.js to index.ts

5fbb27f

- Moved index.ts from root to src/index.ts to align with TypeScript structure - Updated package.json

vershwal force-pushed the typesForUrlUtils branch from ab5c79c to 5fbb27f Compare November 20, 2025 07:43

vershwal marked this pull request as ready for review November 20, 2025 07:52

vershwal changed the title ~~[wip] Types for url utils~~ Added types for url-utils Nov 20, 2025

vershwal requested review from allouis and Copilot and removed request for Copilot November 20, 2025 08:07

Copilot started reviewing on behalf of vershwal November 20, 2025 08:18 View session

Copilot finished reviewing on behalf of vershwal November 20, 2025 08:20

vershwal marked this pull request as draft November 20, 2025 08:22

vershwal marked this pull request as ready for review November 20, 2025 08:22

vershwal requested a review from Copilot November 20, 2025 08:38

Copilot started reviewing on behalf of vershwal November 20, 2025 08:39 View session

Copilot finished reviewing on behalf of vershwal November 20, 2025 08:40

Copilot AI reviewed Nov 20, 2025

View reviewed changes

allouis reviewed Dec 8, 2025

View reviewed changes

cursor bot reviewed Dec 8, 2025

View reviewed changes

packages/url-utils/src/utils/relative-to-transform-ready.ts Outdated Show resolved Hide resolved

vershwal force-pushed the typesForUrlUtils branch from b8cc242 to 201280b Compare December 8, 2025 12:16

cursor bot reviewed Dec 8, 2025

View reviewed changes

		// Unknown route segment - return 'undefined' string
		return 'undefined';

		// Original code passes options as third parameter (itemPath), preserving that behavior
		return utils.relativeToAbsolute(url, this.getSiteUrl(), options \|\| null, undefined);

Uh oh!

Added types for url-utils #685

Are you sure you want to change the base?

Added types for url-utils #685

Uh oh!

Conversation

vershwal commented Nov 12, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor bot commented Nov 12, 2025

Uh oh!

vershwal commented Nov 20, 2025

Uh oh!

vershwal commented Nov 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allouis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

This is the final PR Bugbot will review for you during this billing cycle

Uh oh!

cursor bot Dec 8, 2025

Choose a reason for hiding this comment

Bug: Null regex cast causes runtime error in plaintext transform

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vershwal commented Nov 12, 2025 •

edited by cursor bot

Loading