fix: upgrade got to latest version #675
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThe project is updated to support got v14 (ESM-only) by implementing lazy dynamic imports instead of static requires. Node.js requirement is bumped to >=20, dependencies are updated accordingly, and test assertions are adjusted for got v14's error message format changes. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Module as index.js
participant ensureGot as ensureGot()
participant got as got v14 (ESM)
Client->>Module: Call makeRequest() or graphQL()
alt got not loaded
Module->>ensureGot: ensureGot()
ensureGot->>got: import('got')
got-->>ensureGot: got instance
ensureGot-->>Module: Promise resolves
else got already loaded
ensureGot-->>Module: Promise resolves immediately
end
Module->>Module: Build request options<br/>(timeout: {request: X})
Module->>Module: Convert URL to string<br/>urlObjToString()
Module->>got: got(uriString, options)
got-->>Module: Response
Module-->>Client: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)index.js(5 hunks)package.json(2 hunks)test/shopify.test.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: Generate manual testing checkboxes for the PR. The checkboxes should cover:
- Visual verification that the UI renders correctly
- Functional testing of user interactions
- Edge cases and error states
- Cross-browser/device testing if applicable
- Integration with related features
Files:
package.jsontest/shopify.test.jsindex.js
🧬 Code graph analysis (2)
test/shopify.test.js (1)
index.js (1)
got(13-13)
index.js (3)
test/shopify.test.js (1)
got(28-28)resources/fulfillment-event.js (6)
url(34-34)url(48-48)url(66-66)url(88-88)url(108-108)url(148-148)mixins/base-child.js (7)
url(22-22)url(35-35)url(49-49)url(63-63)url(76-76)url(90-90)url(115-115)
🔇 Additional comments (6)
.gitignore (1)
3-3: Ignore.idea/project files – LGTMGood to keep local IDE metadata out of the repo.
package.json (1)
10-12: Runtime + got upgrade looks consistent; treat as semver‑majorThe Node >=20.0.0 engine requirement and got ^14.6.4 align with the new ESM‑only, dynamic‑import usage in
index.jsand tests. Please ensure:
- This is released as a major version.
- README/CHANGELOG and any install docs clearly call out the new Node 20+ requirement and got major bump.
Please re‑run the full test suite on Node 20+ and confirm there are no compatibility issues with devDependencies (e.g., mocha, eslint, tsd) on that runtime.
Also applies to: 19-23
test/shopify.test.js (1)
27-37: Dynamic got import in tests correctly mirrors ESM usageLazy
import('got')in abeforehook plus attaching the error classes onto the localgotfunction keeps theinstanceofassertions working with the ESM‑only got v14.Please confirm that running
npm testunder Node 20+ exercises these paths (RequestError/TimeoutError/ParseError/HTTPError) and all type checks still pass with got v14.index.js (3)
179-188: Request path: timeout, retry config, and URL string usage align with got v14
- Using
timeout: { request: this.options.timeout }matches got’s object‑based timeout API.- When
maxRetriesis 0,options.retry = { limit: 0 }cleanly disables retries for got v12+.- Converting the
uriobject viaurlObjToString(uri)and callingensureGot().then(() => got(urlString, options))correctly adapts older URL‑object callers to the string‑based got API.- The 202 +
Locationhandling and Link header parsing remain intact and still recurse throughthis.request()with the new URL handling.Please run the REST tests that cover:
- 202 responses with Location headers,
- pagination via
Linkheaders,- and
maxRetries0 vs >0,
to confirm behavior is unchanged with got v14 and the new options shapes.Also applies to: 210-222, 224-263
299-320: GraphQL path: updated timeout/retry and dynamic got usage preserve behavior
- GraphQL requests now share the same
timeout: { request: this.options.timeout }pattern as REST.- Retry configuration uses
{ limit: this.options.maxRetries }or{ limit: 0 }when disabled, which matches the new got retry API.- Converting the GraphQL URI object via
urlObjToString(uri)and routing throughensureGot().then(() => got(urlString, options))keeps existingupdateGraphqlLimits,maybeError, anddecorateErrorlogic intact while satisfying got’s input requirements.Please re‑run the GraphQL tests that cover:
- retry throttling via GraphQL cost limits,
- timeout retries,
- and
beforeError/afterResponsehooks,
to validate they still behave as expected with got v14.Also applies to: 359-371, 373-378
11-15: Got v14 API usage verified and correctThe default export in got v14 remains a callable function accepting
(url, options)and supports bothtimeoutandretryoptions, confirming the code's approach is sound. Dynamicimport('got')from CommonJS is supported on Node 20+, validating the lazy-load pattern used here. No further changes needed.
1 participant
As a part of fixing this SDK's issue with retry mechanism caused by underlying
gotpackage mentioned as here! We need to upgradegotto latest version.Since
gotis nowesmonly sincev12, I updated the package to ensure latestgotis compatible with this SDK. This is breaking change as minimum supported node version is20now!Any edits by maintainer are welcome as I would really love to have this in official release. Until then I will be using my own forked version as a workaround!
Summary by CodeRabbit
Chores
Bug Fixes