Skip to content

Comments

[Lightspeed] Replace node-fetch with global built-in fetch function#2345

Open
michael-valdron wants to merge 13 commits intoredhat-developer:mainfrom
michael-valdron:lightspeed/replace-node-fetch
Open

[Lightspeed] Replace node-fetch with global built-in fetch function#2345
michael-valdron wants to merge 13 commits intoredhat-developer:mainfrom
michael-valdron:lightspeed/replace-node-fetch

Conversation

@michael-valdron
Copy link
Member

@michael-valdron michael-valdron commented Feb 17, 2026

User description

ref: https://issues.redhat.com/browse/RHIDP-12033

Replace node-fetch with built-in fetch

Replaces fetch function with built-in one and refactors source to fit the change. This change comes from ADR014 that now recommends the use of the global built-in fetch function since Node v20.

Hey, I just made a Pull Request!

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

PR Type

Enhancement


Description

  • Replace node-fetch dependency with Node.js built-in fetch function

  • Refactor response piping to use Readable.fromWeb() for stream conversion

  • Update target Node version to 22 and configure Yarn 4.12

  • Add early return in error handling to prevent response piping

  • Remove node-fetch from package dependencies


Diagram Walkthrough

flowchart LR
  A["node-fetch dependency"] -->|Remove| B["Built-in fetch"]
  B -->|Convert stream| C["Readable.fromWeb"]
  C -->|Pipe response| D["Express response"]
  E["Node 18/20"] -->|Update to| F["Node 22"]
Loading

File Walkthrough

Relevant files
Enhancement
router.ts
Replace node-fetch with built-in fetch and refactor piping

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts

  • Remove node-fetch import and use global built-in fetch
  • Add Readable import from node:stream module
  • Convert fetch response body using Readable.fromWeb() for piping
    compatibility
  • Add early return statement after error response to prevent piping
  • Add null check for fetchResponse.body before piping
+8/-2     
Documentation
blue-donuts-wash.md
Changeset documenting node-fetch replacement                         

workspaces/lightspeed/.changeset/blue-donuts-wash.md

  • Document breaking change for lightspeed-backend plugin
  • Provide detailed diff showing removal of node-fetch import
  • Explain response piping refactoring with Readable.fromWeb()
  • Reference ADR014 architecture decision for built-in fetch adoption
+59/-0   
Configuration changes
.yarnrc.yml
Configure Yarn 4.12 for lightspeed workspace                         

workspaces/lightspeed/.yarnrc.yml

  • Create new Yarn configuration file for lightspeed workspace
  • Point to Yarn 4.12.0 release under shared yarn releases directory
+1/-0     
package.json
Update Node version and add dependency                                     

workspaces/lightspeed/package.json

  • Update Node engine requirement from "18 || 20" to "22"
  • Add package.json dependency version ^2.0.1
+2/-1     
Dependencies
package.json
Remove node-fetch dependency                                                         

workspaces/lightspeed/plugins/lightspeed-backend/package.json

  • Remove node-fetch version 2.7.0 from dependencies
  • Keep http-proxy-middleware dependency unchanged
+1/-2     

@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Feb 17, 2026

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-lightspeed-backend workspaces/lightspeed/plugins/lightspeed-backend minor v1.3.0

@michael-valdron
Copy link
Member Author

Added or Updated documentation
Screenshots attached (for UI changes)

N/A

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Feb 17, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🟢
No codebase code duplication found No new components were introduced in the PR code
Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Internal details exposed: The API returns errormsg to the client, which includes upstream-provided error details
and/or thrown error stringification, potentially leaking internal information.

Referred Code
const errormsg = `Error from lightspeed-core server: ${errorBody.error?.message || errorBody?.detail?.cause || 'Unknown error'}`;
logger.error(errormsg);

// Return a 500 status for any upstream error
response.status(500).json({
  error: errormsg,
});

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Fragile error parsing: The upstream error path always attempts fetchResponse.json() which may throw for non-JSON
responses, relying on the catch block without ensuring a consistent, graceful error
response.

Referred Code
  const errorBody = await fetchResponse.json();
  const errormsg = `Error from lightspeed-core server: ${errorBody.error?.message || errorBody?.detail?.cause || 'Unknown error'}`;
  logger.error(errormsg);

  // Return a 500 status for any upstream error
  response.status(500).json({
    error: errormsg,
  });

  return;
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Sensitive data in logs: logger.error(errormsg) logs upstream error content and stringified exceptions which may
include sensitive data depending on what the upstream returns or what error contains.

Referred Code
    logger.error(errormsg);

    // Return a 500 status for any upstream error
    response.status(500).json({
      error: errormsg,
    });

    return;
  }

  // Pipe the response back to the original response
  if (fetchResponse.body) {
    const nodeStream = Readable.fromWeb(fetchResponse.body as any);
    nodeStream.pipe(response);
  }
} catch (error) {
  const errormsg = `Error fetching completions from ${provider}: ${error}`;
  logger.error(errormsg);

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Feb 17, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Guard against missing response body

Handle the case where fetchResponse.body is null by adding an else block to send
a 500 error response, preventing the client from hanging.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts [234-237]

 if (fetchResponse.body) {
   const nodeStream = Readable.fromWeb(fetchResponse.body as any);
   nodeStream.pipe(response);
+  return;
+} else {
+  response.status(500).json({ error: 'Empty response body from upstream' });
+  return;
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly identifies a potential issue where a null response body would cause the client request to hang. Adding an else block to handle this case makes the code more robust.

Medium
Remove unnecessary as any cast

Remove the unnecessary as any type assertion when calling
Readable.fromWeb(fetchResponse.body as any).

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts [235]

-const nodeStream = Readable.fromWeb(fetchResponse.body as any);
+const nodeStream = Readable.fromWeb(fetchResponse.body);
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that the as any cast is unnecessary due to type narrowing from the preceding if check, and removing it improves type safety and code quality.

Low
Possible issue
Remove incorrect package dependency
Suggestion Impact:The commit removed the "package.json": "^2.0.1" entry from the dependencies section in package.json.

code diff:

   "dependencies": {
-    "package.json": "^2.0.1",
     "react": "^18.3.1"

Remove the "package.json": "^2.0.1" dependency from package.json as it appears
to be an erroneous addition.

workspaces/lightspeed/package.json [56-59]

 "dependencies": {
-  "package.json": "^2.0.1",
   "react": "^18.3.1"
 },

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: This suggestion correctly identifies that the "package.json": "^2.0.1" dependency was likely added in error and should be removed to prevent adding an unnecessary and incorrect package to the project.

Low
  • Update

@michael-valdron michael-valdron force-pushed the lightspeed/replace-node-fetch branch 3 times, most recently from e680079 to 78943b2 Compare February 19, 2026 21:32
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
…ugins root

Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
@michael-valdron michael-valdron force-pushed the lightspeed/replace-node-fetch branch from 78943b2 to 31d1517 Compare February 19, 2026 21:40
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant