Updated dependency got to v13.0.0#27073
Merged
kevinansfield merged 1 commit intoTryGhost:mainfrom Apr 2, 2026
Merged
Conversation
Contributor
WalkthroughImports of got were changed to use the module default ( 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/services/oembed/oembed-service.js`:
- Around line 189-191: The request options currently spread the incoming options
directly (variable options) which allows a numeric timeout to overwrite the
default {request: 2000} and break got's required timeout.request shape; before
spreading options (in the function that builds the got request in
oembed-service.js) normalize options.timeout: if typeof options.timeout ===
'number' replace it with {request: options.timeout}, otherwise ensure a fallback
of {request: 2000}; then spread the normalized timeout into the final request
options so got always receives timeout.request.
In
`@ghost/core/core/server/services/recommendations/service/recommendation-metadata-service.ts`:
- Line 20: The oEmbed timeout/options are being dropped on the fallback
call—update every invocation of fetchOembedDataFromUrl that currently calls
fetchOembedDataFromUrl(url.toString(), 'mention') (and the similar call at the
other fallback) to pass the original options object through (e.g.
fetchOembedDataFromUrl(url.toString(), 'mention', options)). Ensure the options
parameter from the surrounding method is threaded into these fallback calls so
the widened public timeout contract is respected by fetchOembedDataFromUrl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c8c8687-e697-48e4-a661-1640f0e03254
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
ghost/core/core/server/lib/request-external.jsghost/core/core/server/services/auth/session/session-service.jsghost/core/core/server/services/members/members-api/services/geolocation-service.jsghost/core/core/server/services/mentions/mention-discovery-service.jsghost/core/core/server/services/mentions/mention-sending-service.jsghost/core/core/server/services/mentions/webmention-metadata.jsghost/core/core/server/services/oembed/oembed-service.jsghost/core/core/server/services/recommendations/service/recommendation-metadata-service.tsghost/core/core/server/services/slack-notifications/slack-notifications.jsghost/core/package.jsonghost/core/test/unit/server/lib/request-external.test.jsghost/core/test/unit/server/services/oembed/oembed-service.test.jsghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js
|
|
||
| type OEmbedService = { | ||
| fetchOembedDataFromUrl<Type extends string>(url: string, type: Type, options?: {timeout?: number}): Promise<OembedMetadata<Type>> | ||
| fetchOembedDataFromUrl<Type extends string>(url: string, type: Type, options?: {timeout?: number | {request: number}}): Promise<OembedMetadata<Type>> |
Contributor
There was a problem hiding this comment.
Don't drop options on the oEmbed fallback.
Lines 20 and 89 widen the public timeout contract, but Line 123 still calls fetchOembedDataFromUrl(url.toString(), 'mention') without options. Non-Ghost sites will therefore ignore the caller's timeout and fall back to the oEmbed service defaults instead.
🛠️ Suggested fix
- const oembed = await this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention');
+ const oembed = await this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention', options);Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/services/recommendations/service/recommendation-metadata-service.ts`
at line 20, The oEmbed timeout/options are being dropped on the fallback
call—update every invocation of fetchOembedDataFromUrl that currently calls
fetchOembedDataFromUrl(url.toString(), 'mention') (and the similar call at the
other fallback) to pass the original options object through (e.g.
fetchOembedDataFromUrl(url.toString(), 'mention', options)). Ensure the options
parameter from the surrounding method is threaded into these fallback calls so
the widened public timeout contract is respected by fetchOembedDataFromUrl.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #27073 +/- ##
=======================================
Coverage 73.19% 73.20%
=======================================
Files 1531 1531
Lines 121796 121832 +36
Branches 14695 14701 +6
=======================================
+ Hits 89154 89189 +35
- Misses 31625 31626 +1
Partials 1017 1017
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kevinansfield
force-pushed
the
upgrade-to-got-13
branch
from
fe5f784 to
0b501e3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/recommendations/service/recommendation-metadata-service.test.ts (1)
6-7: Functional but stylistically inconsistent with the ESM imports above.The change to
require('got').defaultis necessary because got v13 is an ESM-only package, and this is a pragmatic workaround to access the default export in a CommonJS-compatible way. Thesinon.stub(got, 'get')at line 188 will correctly stub the.get()method on the got instance.Consider whether the project should adopt a consistent approach for handling ESM packages across TypeScript test files (e.g., using dynamic imports or updating tsconfig for ESM interop).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/recommendations/service/recommendation-metadata-service.test.ts` around lines 6 - 7, The test currently uses a CommonJS-style import with const got = require('got').default which is functional but inconsistent with the ESM-style imports used elsewhere; update the test to use a consistent ESM-compatible import approach (for example, use dynamic import: const {default: got} = await import('got') at the top of the test setup or enable esModuleInterop and replace the require with an import got from 'got') so that the got variable remains the default-exported instance that sinon.stub(got, 'get') can stub; ensure the chosen approach is applied consistently in this test file and any related tests to keep style uniform.ghost/core/core/server/services/auth/session/session-service.js (1)
312-322: LGTM on the got v13 option shapes, but note the lack of test coverage.The
timeout: { request: 500 }andretry: { limit: 0 }shapes are correct for got v13.However, per the relevant code snippet, the unit test file
session-service.test.jscontains no tests forgetGeolocationFromIP. This means these changes have no test coverage to validate the new option structures work correctly with the geolocation endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/auth/session/session-service.js` around lines 312 - 322, Add unit tests for getGeolocationFromIP in session-service.test.js to cover the new got v13 option shapes: assert that when calling getGeolocationFromIP it constructs got options with timeout: {request: 500} and, when process.env.NODE_ENV starts with 'test', retry: {limit: 0}. Mock/stub the got/http client used in the session service (referencing the getGeolocationFromIP function in session-service.js) to capture the passed options and verify both timeout and retry fields are present and correct, and include at least one test for normal env and one for test env behavior.ghost/core/core/server/services/oembed/oembed-service.js (1)
279-288: Redundant property assignments before spread.Lines 280-282 explicitly assign
hooks,retry, andtimeoutfromrequestOptions, but line 283 immediately spreads...requestOptionswhich overwrites those same properties. The explicit assignments have no effect.♻️ Simplified gotOpts construction
- const gotOpts = { - hooks: requestOptions.hooks, - retry: requestOptions.retry, - timeout: requestOptions.timeout, - ...requestOptions, - headers: { - ...(requestOptions.headers || {}), - 'User-Agent': USER_AGENT - } - }; + const gotOpts = { + ...requestOptions, + headers: { + ...(requestOptions.headers || {}), + 'User-Agent': USER_AGENT + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/oembed/oembed-service.js` around lines 279 - 288, The gotOpts construction redundantly assigns hooks/retry/timeout from requestOptions and then immediately spreads ...requestOptions which overwrites them; fix by either removing the explicit assignments (hooks, retry, timeout) and only using ...requestOptions plus the headers override, or move the ...requestOptions spread earlier and then set the overrides (hooks, retry, timeout, headers with User-Agent) after the spread so the intended overrides take effect; modify the gotOpts object where it is defined (identifier: gotOpts) to avoid duplicated properties while preserving the headers merge that injects USER_AGENT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/auth/session/session-service.js`:
- Around line 312-322: Add unit tests for getGeolocationFromIP in
session-service.test.js to cover the new got v13 option shapes: assert that when
calling getGeolocationFromIP it constructs got options with timeout: {request:
500} and, when process.env.NODE_ENV starts with 'test', retry: {limit: 0}.
Mock/stub the got/http client used in the session service (referencing the
getGeolocationFromIP function in session-service.js) to capture the passed
options and verify both timeout and retry fields are present and correct, and
include at least one test for normal env and one for test env behavior.
In `@ghost/core/core/server/services/oembed/oembed-service.js`:
- Around line 279-288: The gotOpts construction redundantly assigns
hooks/retry/timeout from requestOptions and then immediately spreads
...requestOptions which overwrites them; fix by either removing the explicit
assignments (hooks, retry, timeout) and only using ...requestOptions plus the
headers override, or move the ...requestOptions spread earlier and then set the
overrides (hooks, retry, timeout, headers with User-Agent) after the spread so
the intended overrides take effect; modify the gotOpts object where it is
defined (identifier: gotOpts) to avoid duplicated properties while preserving
the headers merge that injects USER_AGENT.
In
`@ghost/core/test/unit/server/services/recommendations/service/recommendation-metadata-service.test.ts`:
- Around line 6-7: The test currently uses a CommonJS-style import with const
got = require('got').default which is functional but inconsistent with the
ESM-style imports used elsewhere; update the test to use a consistent
ESM-compatible import approach (for example, use dynamic import: const {default:
got} = await import('got') at the top of the test setup or enable
esModuleInterop and replace the require with an import got from 'got') so that
the got variable remains the default-exported instance that sinon.stub(got,
'get') can stub; ensure the chosen approach is applied consistently in this test
file and any related tests to keep style uniform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af50ba48-f50e-4448-bebc-b181ba01b527
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
ghost/core/core/server/lib/request-external.jsghost/core/core/server/services/auth/session/session-service.jsghost/core/core/server/services/members/members-api/services/geolocation-service.jsghost/core/core/server/services/mentions/mention-discovery-service.jsghost/core/core/server/services/mentions/mention-sending-service.jsghost/core/core/server/services/mentions/webmention-metadata.jsghost/core/core/server/services/oembed/oembed-service.jsghost/core/core/server/services/recommendations/service/recommendation-metadata-service.tsghost/core/core/server/services/slack-notifications/slack-notifications.jsghost/core/package.jsonghost/core/test/unit/server/lib/request-external.test.jsghost/core/test/unit/server/services/oembed/oembed-service.test.jsghost/core/test/unit/server/services/recommendations/service/recommendation-metadata-service.test.tsghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js
✅ Files skipped from review due to trivial changes (3)
- ghost/core/core/server/services/mentions/mention-sending-service.js
- ghost/core/core/server/services/mentions/mention-discovery-service.js
- ghost/core/core/server/services/mentions/webmention-metadata.js
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/package.json
- ghost/core/test/unit/server/services/oembed/oembed-service.test.js
- ghost/core/core/server/services/slack-notifications/slack-notifications.js
- ghost/core/core/server/lib/request-external.js
- ghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js
- ghost/core/core/server/services/recommendations/service/recommendation-metadata-service.ts
- ghost/core/core/server/services/members/members-api/services/geolocation-service.js
kevinansfield
force-pushed
the
upgrade-to-got-13
branch
from
0b501e3 to
be3ef91
Compare
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/recommendations/service/recommendation-metadata-service.ts (1)
123-123:⚠️ Potential issue | 🟡 MinorOptions are not passed to the oEmbed fallback call.
The
fetchmethod accepts anoptionsparameter with timeout configuration, and it correctly passesoptionsto#fetchJSONcalls (lines 97, 104). However, when falling back tofetchOembedDataFromUrlfor non-Ghost sites, theoptionsare not passed. This means the caller's timeout preference is ignored, and the oEmbed service will use its internal defaults instead.🔧 Suggested fix
- const oembed = await this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention'); + const oembed = await this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention', options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/recommendations/service/recommendation-metadata-service.ts` at line 123, The fallback call to the oEmbed service is not forwarding the caller's timeout/options so caller preferences are ignored; update the call in recommendation-metadata-service.ts from this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention') to include the options parameter (e.g. this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention', options)), ensuring the options object used by fetch(...) and `#fetchJSON` is propagated to fetchOembedDataFromUrl so timeouts and other settings are honored.
🧹 Nitpick comments (2)
ghost/core/core/server/services/oembed/oembed-service.js (2)
278-288: Spread order may cause unintended overwrites.The explicit properties
hooks,retry, andtimeoutare set first, then...requestOptionsis spread which may overwrite them with whatever is inrequestOptions. If this is intentional (to allow defaults to pull through from externalRequest), consider documenting this behavior. Otherwise, move the spread before the explicit assignments.Additionally,
requestOptions.hooksmight contain the init hooks (includingdisableRetriesin test mode), but spreadingrequestOptionsafter explicitly settinghooksmeans the explicit assignment is overwritten.💡 Suggested reorder if explicit values should take precedence
const gotOpts = { + ...requestOptions, hooks: requestOptions.hooks, retry: requestOptions.retry, timeout: requestOptions.timeout, - ...requestOptions, headers: { ...(requestOptions.headers || {}), 'User-Agent': USER_AGENT } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/oembed/oembed-service.js` around lines 278 - 288, The spread order in the gotOpts construction causes explicit properties (hooks, retry, timeout) to be overwritten by requestOptions; update the gotOpts build so that ...requestOptions is spread first and then explicitly set hooks, retry, timeout and headers (merging requestOptions.headers with 'User-Agent') so the explicit values take precedence; ensure you reference requestOptions, gotOpts, externalRequest and USER_AGENT when making the change and preserve merging of headers to include any existing headers plus the USER_AGENT.
290-294: Test-mode retry override may be incomplete.The
disableRetrieshook inrequest-external.js(lines 205-214) sets bothlimit: 0andcalculateDelay: () => 0. Here, onlylimit: 0is set. Whilelimit: 0should prevent retries, the discrepancy could cause subtle differences in behavior between the hook-based and explicit retry configurations.Consider aligning with the full retry shape:
🔧 Align with disableRetries hook
if (process.env.NODE_ENV?.startsWith('test')) { gotOpts.retry = { - limit: 0 + limit: 0, + calculateDelay: () => 0 }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/oembed/oembed-service.js` around lines 290 - 294, The test-mode retry override in oembed-service.js only sets gotOpts.retry.limit = 0 which differs from the disableRetries hook in request-external.js; update the test branch where you modify gotOpts (the object used for got requests in the oembed service) to set the full retry shape (e.g., gotOpts.retry = { limit: 0, calculateDelay: () => 0 }) so behavior matches the disableRetries hook exactly and avoids any subtle differences in retry timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@ghost/core/core/server/services/recommendations/service/recommendation-metadata-service.ts`:
- Line 123: The fallback call to the oEmbed service is not forwarding the
caller's timeout/options so caller preferences are ignored; update the call in
recommendation-metadata-service.ts from
this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention') to include
the options parameter (e.g.
this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention', options)),
ensuring the options object used by fetch(...) and `#fetchJSON` is propagated to
fetchOembedDataFromUrl so timeouts and other settings are honored.
---
Nitpick comments:
In `@ghost/core/core/server/services/oembed/oembed-service.js`:
- Around line 278-288: The spread order in the gotOpts construction causes
explicit properties (hooks, retry, timeout) to be overwritten by requestOptions;
update the gotOpts build so that ...requestOptions is spread first and then
explicitly set hooks, retry, timeout and headers (merging requestOptions.headers
with 'User-Agent') so the explicit values take precedence; ensure you reference
requestOptions, gotOpts, externalRequest and USER_AGENT when making the change
and preserve merging of headers to include any existing headers plus the
USER_AGENT.
- Around line 290-294: The test-mode retry override in oembed-service.js only
sets gotOpts.retry.limit = 0 which differs from the disableRetries hook in
request-external.js; update the test branch where you modify gotOpts (the object
used for got requests in the oembed service) to set the full retry shape (e.g.,
gotOpts.retry = { limit: 0, calculateDelay: () => 0 }) so behavior matches the
disableRetries hook exactly and avoids any subtle differences in retry timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 665e9372-c80e-4d0f-94f1-5d477244e196
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
ghost/core/core/server/lib/request-external.jsghost/core/core/server/services/auth/session/session-service.jsghost/core/core/server/services/members/members-api/services/geolocation-service.jsghost/core/core/server/services/mentions/mention-discovery-service.jsghost/core/core/server/services/mentions/mention-sending-service.jsghost/core/core/server/services/mentions/webmention-metadata.jsghost/core/core/server/services/oembed/oembed-service.jsghost/core/core/server/services/recommendations/service/recommendation-metadata-service.tsghost/core/core/server/services/slack-notifications/slack-notifications.jsghost/core/package.jsonghost/core/test/unit/server/lib/request-external.test.jsghost/core/test/unit/server/services/oembed/oembed-service.test.jsghost/core/test/unit/server/services/recommendations/service/recommendation-metadata-service.test.tsghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js
✅ Files skipped from review due to trivial changes (5)
- ghost/core/core/server/services/mentions/mention-discovery-service.js
- ghost/core/test/unit/server/services/recommendations/service/recommendation-metadata-service.test.ts
- ghost/core/test/unit/server/services/oembed/oembed-service.test.js
- ghost/core/core/server/services/mentions/mention-sending-service.js
- ghost/core/core/server/services/auth/session/session-service.js
🚧 Files skipped from review as they are similar to previous changes (6)
- ghost/core/package.json
- ghost/core/test/unit/server/lib/request-external.test.js
- ghost/core/core/server/services/members/members-api/services/geolocation-service.js
- ghost/core/core/server/services/slack-notifications/slack-notifications.js
- ghost/core/core/server/lib/request-external.js
- ghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
no issue
@tryghost/requestuses v13.0.0 but Ghost's direct usage ofgotwas on an earlier v11.x