Skip to content

🏗 Replaced remaining dynamic ESM imports with require() - #954

Merged
acburdine merged 1 commit into
mainfrom
cleanup/dynamic-esm-imports
Sep 1, 2026
Merged

🏗 Replaced remaining dynamic ESM imports with require()#954
acburdine merged 1 commit into
mainfrom
cleanup/dynamic-esm-imports

Conversation

@acburdine

Copy link
Copy Markdown
Member

Follow-up to #953.

JobManager and WebhookMockReceiver both loaded p-wait-for via await import() inside a method — a leftover from before Node supported require() of ESM.

Unlike @tryghost/request, neither had the actual bug: they await at the call site instead of caching a module-scope promise, so the module is never left half-loaded across arbitrary consumer code. The window is just the import's own I/O, and nothing in the repo requires p-wait-for synchronously. This is cleanup, not a fix — it removes the last of the pattern from library code.

Changes

  • Load p-wait-for with require() at module scope in job-manager and webhook-mock-receiver
  • Declare "engines": { "node": "^22.12.0 || >=24.0.0" } on both, matching 🐛 Fixed dynamic got import race on Node 24 #953require(esm) was flagged before 22.12
  • Align got on 15.1.0 in webhook-mock-receiver to match @tryghost/request, and drop the await import('got') in its tests

got is a test-only dependency in webhook-mock-receiver, so the bump carries no runtime risk. Left it as a literal version rather than moving both packages to a catalog: entry — nx release publish shells out to npm, and I didn't want to bet a published runtime dependency on the catalog: protocol being rewritten at pack time.

Testing

Both suites pass on Node 22 and Node 24: job-manager 40 passed (1 todo), webhook-mock-receiver 8 passed at 100% coverage. Lint and format clean.

🤖 Generated with Claude Code

refs #953

Follow-up to the `@tryghost/request` fix. `JobManager` and
`WebhookMockReceiver` both loaded `p-wait-for` via `await import()` inside
a method, which dates from before Node supported `require()` of ESM.

These awaited at the call site rather than caching a module-scope promise,
so they never had the wide race window that broke `@tryghost/request` on
Node 24. Converting them is cleanup rather than a fix, but it removes the
last of the pattern from library code.

- Load `p-wait-for` with `require()` at module scope in both packages
- Declare the `^22.12.0 || >=24.0.0` engines floor that `require(esm)` needs
- Align `got` on 15.1.0 in webhook-mock-receiver to match `@tryghost/request`
  (test-only dependency there) and drop its `await import('got')` in tests

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 19 days. After that, they cost $0.25 per reviewed file.

Or wait 41 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 106ed42b-36f6-4c8c-ab99-cfe47a49a0ec

📥 Commits

Reviewing files that changed from the base of the PR and between 8d49445 and a71f2c6.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • packages/job-manager/lib/JobManager.js
  • packages/job-manager/package.json
  • packages/webhook-mock-receiver/lib/WebhookMockReceiver.js
  • packages/webhook-mock-receiver/package.json
  • packages/webhook-mock-receiver/test/WebhookMockReceiver.test.js

Comment @coderabbitai help to get the list of available commands.

@acburdine
acburdine merged commit 7f893aa into main Sep 1, 2026
7 checks passed
@acburdine
acburdine deleted the cleanup/dynamic-esm-imports branch September 1, 2026 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant