Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support use on Node.js 16 #550

Merged
merged 2 commits into from
Dec 2, 2024
Merged

fix: support use on Node.js 16 #550

merged 2 commits into from
Dec 2, 2024

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Dec 2, 2024

Summary by CodeRabbit

  • New Features

    • Enhanced compatibility with Node.js 16 through the addition of new utility functions.
    • Introduced global definitions for ReadableStream, Blob, and DOMException to support newer features.
  • Bug Fixes

    • Maintained error handling in JSON parsing to ensure detailed error messages.
  • Documentation

    • Updated comments and documentation to reflect new utility functions and their purposes.

Copy link

coderabbitai bot commented Dec 2, 2024

Walkthrough

The changes involve modifications to two files: src/index.ts and src/utils.ts. In src/index.ts, a new import statement for the patchForNode16 function is added, which is invoked immediately to modify the module's initialization sequence. In src/utils.ts, new imports and functions are introduced to enhance compatibility with Node.js 16, including the addition of patchForNode16 and getDOMExceptionClass. The overall functionality of existing methods remains unchanged, with no alterations to exported entities.

Changes

File Change Summary
src/index.ts Added import for patchForNode16 and invoked it to modify initialization sequence.
src/utils.ts Added imports for ReadableStream and Blob, added functions patchForNode16 and getDOMExceptionClass, and repositioned convertHeader. Existing functions remain unchanged.

Possibly related PRs

  • test: run test on Node.js 23 #543: The main PR introduces the patchForNode16 function to enhance compatibility with Node.js 16, while the retrieved PR updates the CI configuration to support Node.js 23, indicating a broader effort to maintain compatibility across Node.js versions.

Poem

🐰 In the code where bunnies play,
New patches hop in bright array.
With streams and blobs, we leap and bound,
In Node's embrace, new joys are found!
So let us cheer, with hearts so light,
For every change makes coding bright! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

pkg-pr-new bot commented Dec 2, 2024

Open in Stackblitz

npm i https://pkg.pr.new/node-modules/urllib@550

commit: 39bba2c

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
src/utils.ts (2)

212-225: Consider adding Node.js version check

The patch function should verify it's running on Node.js 16 to avoid potential conflicts with existing globals in other versions.

 export function patchForNode16() {
+  // Only patch if running on Node.js 16
+  if (process.version.startsWith('v16.')) {
     if (typeof global.ReadableStream === 'undefined') {
       // @ts-expect-error
       global.ReadableStream = ReadableStream;
     }
     if (typeof global.Blob === 'undefined') {
       // @ts-expect-error
       global.Blob = Blob;
     }
     if (typeof global.DOMException === 'undefined') {
       // @ts-expect-error
       global.DOMException = getDOMExceptionClass();
     }
+  }
 }
🧰 Tools
🪛 eslint

[error] 214-214: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 218-218: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 222-222: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)

🪛 GitHub Check: Node.js / Test (windows-latest, 23)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 22)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 20)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 23)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 22)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 20)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


228-235: Improve error handling in getDOMExceptionClass

The function relies on atob() being available and throwing a DOMException. Consider adding a fallback mechanism.

 function getDOMExceptionClass() {
   try {
-    // @ts-ignore
+    // @ts-expect-error
     atob(0);
   } catch (err: any) {
     return err.constructor;
   }
+  // Fallback if atob doesn't throw or isn't available
+  return class DOMException extends Error {
+    constructor(message: string, name = 'Error') {
+      super(message);
+      this.name = name;
+    }
+  };
 }
🧰 Tools
🪛 eslint

[error] 230-230: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)

🪛 GitHub Check: Node.js / Test (windows-latest, 23)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 22)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 20)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 23)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 22)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 20)

[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e030ec9 and 62d3a95.

📒 Files selected for processing (2)
  • src/index.ts (1 hunks)
  • src/utils.ts (2 hunks)
🧰 Additional context used
🪛 eslint
src/utils.ts

[error] 214-214: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 218-218: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 222-222: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 230-230: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)

🪛 GitHub Check: Node.js / Test (windows-latest, 23)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 22)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 20)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 23)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 22)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 20)
src/utils.ts

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 230-230:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🔇 Additional comments (1)
src/utils.ts (1)

4-5: LGTM! Proper usage of built-in Node.js modules

Good use of the node: protocol prefix for importing built-in modules.

src/index.ts Show resolved Hide resolved
src/utils.ts Outdated
Comment on lines 212 to 225
export function patchForNode16() {
if (typeof global.ReadableStream === 'undefined') {
// @ts-ignore
global.ReadableStream = ReadableStream;
}
if (typeof global.Blob === 'undefined') {
// @ts-ignore
global.Blob = Blob;
}
if (typeof global.DOMException === 'undefined') {
// @ts-ignore
global.DOMException = getDOMExceptionClass();
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace @ts-ignore with @ts-expect-error

The static analysis tools correctly flag the usage of @ts-ignore. Using @ts-expect-error is preferred as it will error if the type error is fixed, preventing stale suppressions.

-    // @ts-ignore
+    // @ts-expect-error
     global.ReadableStream = ReadableStream;
-    // @ts-ignore
+    // @ts-expect-error
     global.Blob = Blob;
-    // @ts-ignore
+    // @ts-expect-error
     global.DOMException = getDOMExceptionClass();
📝 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.

Suggested change
export function patchForNode16() {
if (typeof global.ReadableStream === 'undefined') {
// @ts-ignore
global.ReadableStream = ReadableStream;
}
if (typeof global.Blob === 'undefined') {
// @ts-ignore
global.Blob = Blob;
}
if (typeof global.DOMException === 'undefined') {
// @ts-ignore
global.DOMException = getDOMExceptionClass();
}
}
export function patchForNode16() {
if (typeof global.ReadableStream === 'undefined') {
// @ts-expect-error
global.ReadableStream = ReadableStream;
}
if (typeof global.Blob === 'undefined') {
// @ts-expect-error
global.Blob = Blob;
}
if (typeof global.DOMException === 'undefined') {
// @ts-expect-error
global.DOMException = getDOMExceptionClass();
}
}
🧰 Tools
🪛 eslint

[error] 214-214: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 218-218: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)


[error] 222-222: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.

(@typescript-eslint/ban-ts-comment)

🪛 GitHub Check: Node.js / Test (windows-latest, 23)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 22)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 20)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (windows-latest, 18)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 23)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 22)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 18)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

🪛 GitHub Check: Node.js / Test (macos-latest, 20)

[failure] 214-214:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 218-218:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free


[failure] 222-222:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes missing coverage. Please review.

Project coverage is 93.50%. Comparing base (9803c4e) to head (39bba2c).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/utils.ts 35.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
- Coverage   94.42%   93.50%   -0.93%     
==========================================
  Files          10       10              
  Lines        1255     1277      +22     
  Branches      285      289       +4     
==========================================
+ Hits         1185     1194       +9     
- Misses         66       79      +13     
  Partials        4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
src/utils.ts (1)

231-239: Improve error handling and documentation

While the implementation cleverly uses atob to get the DOMException class, consider these improvements:

  1. Add JSDoc to explain the implementation strategy
  2. Add error handling for environments where atob might not be available
+ /**
+  * Gets the native DOMException class by triggering an error with atob.
+  * This approach is based on the node-domexception library.
+  * @see https://github.com/jimmywarting/node-domexception
+  * @returns The DOMException class
+  * @throws If atob is not available
+  */
 function getDOMExceptionClass() {
+  if (typeof atob !== 'function') {
+    throw new Error('atob is required to get DOMException class');
+  }
   try {
     // eslint-disable-next-line @typescript-eslint/ban-ts-comment
     // @ts-ignore
     atob(0);
   } catch (err: any) {
     return err.constructor;
   }
+  // This should never happen as atob(0) should always throw
+  throw new Error('Failed to get DOMException class');
 }
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 231-232: src/utils.ts#L231-L232
Added lines #L231 - L232 were not covered by tests


[warning] 235-239: src/utils.ts#L235-L239
Added lines #L235 - L239 were not covered by tests

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 62d3a95 and 39bba2c.

📒 Files selected for processing (1)
  • src/utils.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/utils.ts

[warning] 216-217: src/utils.ts#L216-L217
Added lines #L216 - L217 were not covered by tests


[warning] 221-222: src/utils.ts#L221-L222
Added lines #L221 - L222 were not covered by tests


[warning] 226-227: src/utils.ts#L226-L227
Added lines #L226 - L227 were not covered by tests


[warning] 231-232: src/utils.ts#L231-L232
Added lines #L231 - L232 were not covered by tests


[warning] 235-239: src/utils.ts#L235-L239
Added lines #L235 - L239 were not covered by tests

🔇 Additional comments (3)
src/utils.ts (3)

4-5: LGTM! Imports are correctly specified.

The imports use the recommended node: protocol prefix and import from official Node.js modules.


214-226: Replace @ts-ignore with @ts-expect-error

The static analysis tools correctly flag the usage of @ts-ignore.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 216-217: src/utils.ts#L216-L217
Added lines #L216 - L217 were not covered by tests


[warning] 221-222: src/utils.ts#L221-L222
Added lines #L221 - L222 were not covered by tests


212-228: Verify the impact of global modifications

The function modifies the global scope which could have unintended side effects.

✅ Verification successful

Let me gather more context about the usage of this function and Node.js version handling.


Based on the gathered information, I can now provide a final response:

Global modifications are safely scoped to Node.js 16 compatibility

The patchForNode16() function is a controlled polyfill that only adds missing Web API globals when they're undefined. This is safe because:

  • The package explicitly requires Node.js >= 18.19.0 in package.json
  • The polyfills are only applied when the globals don't exist (defensive checks)
  • These modifications only occur once at initialization (imported and called once in index.ts)
  • No other files modify these globals, preventing conflicts
  • The polyfills are standard Web APIs needed for compatibility
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other global modifications and potential conflicts

# Search for other global.ReadableStream assignments
rg -l "global\.ReadableStream\s*=" --type ts

# Search for other global.Blob assignments
rg -l "global\.Blob\s*=" --type ts

# Search for other global.DOMException assignments
rg -l "global\.DOMException\s*=" --type ts

# Look for potential version checks or Node.js version specific code
rg "process\.version" --type ts

Length of output: 547


Script:

#!/bin/bash
# Check where patchForNode16 is used and how it's related to version checks
rg "patchForNode16" -B 2 -A 2 --type ts

# Check package.json for Node.js version requirements
cat package.json | jq '.engines'

# Look for any test cases or documentation about Node.js compatibility
fd README.md --exec cat {}

Length of output: 16425

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 216-217: src/utils.ts#L216-L217
Added lines #L216 - L217 were not covered by tests


[warning] 221-222: src/utils.ts#L221-L222
Added lines #L221 - L222 were not covered by tests


[warning] 226-227: src/utils.ts#L226-L227
Added lines #L226 - L227 were not covered by tests

Comment on lines +212 to +228
export function patchForNode16() {
if (typeof global.ReadableStream === 'undefined') {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
global.ReadableStream = ReadableStream;
}
if (typeof global.Blob === 'undefined') {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
global.Blob = Blob;
}
if (typeof global.DOMException === 'undefined') {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
global.DOMException = getDOMExceptionClass();
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add test coverage for Node.js 16 compatibility patches

The new patchForNode16 function lacks test coverage. Please add tests to verify:

  1. Polyfills are correctly applied when globals are undefined
  2. Existing globals are preserved when already defined

Would you like me to help create test cases for this function?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 216-217: src/utils.ts#L216-L217
Added lines #L216 - L217 were not covered by tests


[warning] 221-222: src/utils.ts#L221-L222
Added lines #L221 - L222 were not covered by tests


[warning] 226-227: src/utils.ts#L226-L227
Added lines #L226 - L227 were not covered by tests

Comment on lines +231 to +239
function getDOMExceptionClass() {
try {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
atob(0);
} catch (err: any) {
return err.constructor;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add test coverage for DOMException class retrieval

The getDOMExceptionClass function lacks test coverage. Please add tests to verify:

  1. The function returns the correct DOMException class
  2. Error handling for environments without atob

Would you like me to help create test cases for this function?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 231-232: src/utils.ts#L231-L232
Added lines #L231 - L232 were not covered by tests


[warning] 235-239: src/utils.ts#L235-L239
Added lines #L235 - L239 were not covered by tests

@fengmk2 fengmk2 merged commit 78e1336 into master Dec 2, 2024
21 of 23 checks passed
@fengmk2 fengmk2 deleted the support-node-16 branch December 2, 2024 07:15
fengmk2 pushed a commit that referenced this pull request Dec 2, 2024
[skip ci]

## [4.5.1](v4.5.0...v4.5.1) (2024-12-02)

### Bug Fixes

* support use on Node.js 16 ([#550](#550)) ([78e1336](78e1336))
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