Conversation
📝 WalkthroughWalkthroughPinned @wordpress/api-fetch devDependency in package.json to version 7.29 and relaxed the TypeScript type of the fetch helper in BaseProvider from APIFetchOptions to APIFetchOptions, removing a type assertion and a related comment. No other logic changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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 |
acicovic
deleted the
fix/downgrade-wordpress-api-fetch-package-to-7-29
branch
…api-fetch-package-to-7-29" (5c409af)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/content-helper/common/providers/base-provider.tsx (1)
174-181: Possible crash on error path due to unsafe property access.If wpError.message is an object but not an array,
wpError.message[0].msgthrows before the && short-circuits.Apply:
- let errorMessage = wpError.message; - // The error message might be an object with multiple messages. - if ( typeof wpError.message === 'object' && wpError.message[ 0 ].msg ) { - errorMessage = wpError.message[ 0 ].msg; - } + let errorMessage = ''; + if ( typeof wpError?.message === 'string' ) { + errorMessage = wpError.message; + } else if ( Array.isArray( wpError?.message ) && wpError.message[0]?.msg ) { + errorMessage = String( wpError.message[0].msg ); + } else if ( typeof wpError?.message?.message === 'string' ) { + errorMessage = wpError.message.message; + } else { + errorMessage = __( 'An unknown error occurred.', 'wp-parsely' ); + } - return Promise.reject( new ContentHelperError( errorMessage, wpError.code ) ); + return Promise.reject( new ContentHelperError( errorMessage, wpError?.code ) );
🧹 Nitpick comments (3)
src/content-helper/common/providers/base-provider.tsx (3)
139-145: JSDoc return type is incorrect; add a @SInCE entry for this change and fix @param notation.
- The method returns
Promise<T>, notPromise<ContentHelperAPIResponse<any>>.- WordPress JSDoc prefers optional params as
[id]and types without the?suffix.- Add a new @SInCE noting the signature/type change tied to the next plugin version.
Apply:
/** * This method is a wrapper around apiFetch() that automatically adds the * AbortController signal. * - * @since 3.15.0 + * @since 3.15.0 + * @since 3.20.7 Changed to accept APIFetchOptions (no generic) and to always enforce parse: true. * - * @param {APIFetchOptions} options The options to pass to apiFetch. - * @param {string?} id The (optional) ID of the request. + * @param {APIFetchOptions} options The options to pass to apiFetch. + * @param {string} [id] The optional ID of the request. * - * @return {Promise<ContentHelperAPIResponse<any>>} The fetched data. + * @return {Promise<T>} The fetched data. */
182-183: Comment nit: clarify lifecycle.This runs in finally, not only on success.
Apply:
- // Clean-up the AbortController after a successful request. + // Clean up the AbortController after the request completes.
65-66: JSDoc optional param style.Use
[id]with{string}(no?) per WP JSDoc conventions.Apply:
- * @param {string?} id The (optional) ID of the request to cancel. + * @param {string} [id] The optional ID of the request to cancel.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/content-helper/common/providers/base-provider.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
Files:
src/content-helper/common/providers/base-provider.tsx
🧬 Code graph analysis (1)
src/content-helper/common/providers/base-provider.tsx (1)
src/content-helper/common/providers/base-wordpress-provider.tsx (1)
apiFetch(145-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E against WordPress latest
- GitHub Check: build
🔇 Additional comments (1)
src/content-helper/common/providers/base-provider.tsx (1)
146-152: Approve code changes
Noparse:falseusages detected in anyfetchcall sites; downgrade is safe.
| "@types/wordpress__wordcount": "^2.4.5", | ||
| "@typescript-eslint/eslint-plugin": "^6.21.0", | ||
| "@wordpress/api-fetch": "^7.30.0", | ||
| "@wordpress/api-fetch": "7.29", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin looks good; consider forcing a single version across the tree and documenting why.
- To avoid a second copy being hoisted by transitive deps, consider pinning via overrides/resolutions as well and add a rationale comment (similar to others) to prevent accidental upgrades back to 7.30+.
Apply:
"devDependencies": {
- "@wordpress/api-fetch": "7.29",
+ "@wordpress/api-fetch": "7.29",
},
+ "devDependenciesComments": {
+ "@wordpress/api-fetch": "Pinned to 7.29 to avoid stricter typing in 7.30 that breaks our build. Do not upgrade until types are refactored."
+ },
"overrides": {
- "puppeteer-core": "^23.1.0"
+ "puppeteer-core": "^23.1.0",
+ "@wordpress/api-fetch": "7.29"
},Verify no other copy of api-fetch is brought in and no lingering APIFetchOptions remain:
🏁 Script executed:
#!/bin/bash
# Look for any usage of the removed generic.
rg -nP --type=ts --type=tsx 'APIFetchOptions\s*<\s*true\s*>' -C2
# If a lockfile exists, ensure only 7.29 is resolved.
fd -tf 'package-lock.json|pnpm-lock.yaml|yarn.lock' | xargs -I{} rg -n 'api-fetch@|@wordpress/api-fetch' -n {}Length of output: 2630
Force single @wordpress/api-fetch version and document rationale
Multiple transitive deps still pull in @wordpress/api-fetch@7.30.x and @wordpress/api-fetch@6.55.x; add a Yarn resolutions block to pin all instances to 7.29.0 and include a comment:
"resolutions": {
- // existing overrides...
+ "@wordpress/api-fetch": "7.29.0"
},
+ "devDependenciesComments": {
+ "@wordpress/api-fetch": "Pinned to 7.29.0 to avoid stricter typing in 7.30+ that breaks our build"
+ }No occurrences of APIFetchOptions<true> remain.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package.json around line 57, force all transitive instances of
@wordpress/api-fetch to 7.29.0 by adding a Yarn "resolutions" block that pins
"@wordpress/api-fetch" to "7.29.0" and include a one-line comment above it
explaining the rationale ("pin @wordpress/api-fetch to 7.29.0 to avoid mixed
incompatible versions across transitive deps"); after adding the resolution, run
yarn install to update the lockfile and verify no occurrences of
APIFetchOptions<true> remain.
| protected async fetch<T>( options: APIFetchOptions, id?: string ): Promise<T> { | ||
| const { abortController, abortId } = this.getOrCreateController( id ); | ||
| options.signal = abortController.signal; | ||
|
|
||
| try { | ||
| const response = await apiFetch<ContentHelperAPIResponse<T>>( | ||
| options as APIFetchOptions<true> | ||
| ); | ||
| const response = await apiFetch<ContentHelperAPIResponse<T>>( options ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type relaxation reintroduces a runtime hazard; enforce parse=true and avoid mutating caller options.
By dropping APIFetchOptions, callers can now pass parse:false and this method would still treat the result as parsed JSON, causing runtime errors. Also, mutating options in place can surprise callers.
Apply:
- protected async fetch<T>( options: APIFetchOptions, id?: string ): Promise<T> {
+ protected async fetch<T>( options: APIFetchOptions, id?: string ): Promise<T> {
const { abortController, abortId } = this.getOrCreateController( id );
-
- options.signal = abortController.signal;
+ // Enforce JSON parsing and avoid mutating the caller-provided object.
+ const fetchOptions: APIFetchOptions = {
+ ...options,
+ signal: abortController.signal,
+ parse: true,
+ };
try {
- const response = await apiFetch<ContentHelperAPIResponse<T>>( options );
+ const response = await apiFetch<ContentHelperAPIResponse<T>>( fetchOptions );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected async fetch<T>( options: APIFetchOptions, id?: string ): Promise<T> { | |
| const { abortController, abortId } = this.getOrCreateController( id ); | |
| options.signal = abortController.signal; | |
| try { | |
| const response = await apiFetch<ContentHelperAPIResponse<T>>( | |
| options as APIFetchOptions<true> | |
| ); | |
| const response = await apiFetch<ContentHelperAPIResponse<T>>( options ); | |
| protected async fetch<T>( options: APIFetchOptions, id?: string ): Promise<T> { | |
| const { abortController, abortId } = this.getOrCreateController( id ); | |
| // Enforce JSON parsing and avoid mutating the caller-provided object. | |
| const fetchOptions: APIFetchOptions = { | |
| ...options, | |
| signal: abortController.signal, | |
| parse: true, | |
| }; | |
| try { | |
| const response = await apiFetch<ContentHelperAPIResponse<T>>( fetchOptions ); |
🤖 Prompt for AI Agents
In src/content-helper/common/providers/base-provider.tsx around lines 146 to
152, the method relaxes APIFetchOptions allowing callers to pass parse:false and
then mutates their options, which can cause runtime parse errors; change the
method signature to accept APIFetchOptions<true> (enforcing parse=true), stop
mutating the caller's object by creating a shallow copy of options and set
parse:true on the copy (and preserve other fields), and use that copy when
calling apiFetch so the caller's original options remain unchanged.
1 participant
Description
We're having build issues on the develop branch after updating
@wordpress/api-fetchfrom7.29to7.30. With this PR we're downgrading to7.29in an attempt to avoid the issue introduced due to the stricter typing.Summary by CodeRabbit
Chores
Refactor
No user-facing changes in this release; improvements focus on development stability and maintainability.