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

refactor(cli): using built-in fetch instead of axios #1730

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Conversation

ysfscream
Copy link
Member

@ysfscream ysfscream commented Jul 31, 2024

What is the current behavior?

Currently, MQTTX CLI uses Axios for HTTP requests. This introduces an additional dependency that is not heavily utilized within the project.

Issue Number

N/A (or reference the relevant issue if one exists)

What is the new behavior?

This PR replaces Axios with the built-in Fetch API for HTTP requests in MQTTX CLI. This change aims to:

  1. Reduce project dependencies
  2. Decrease bundle size
  3. Potentially improve performance for HTTP operations
  4. Utilize modern, native JavaScript features

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Node.js 18 supports the built-in fetch API.

Important note regarding TypeScript configuration:

Using fetch here with adding DOM lib in tsconfig.json is not correct. See: DefinitelyTyped/DefinitelyTyped#60924. Update to Node.js 20 to resolve fetch global type issue.

To properly implement this change, we need to ensure that the project is using Node.js version 20 or later, which includes proper type definitions for the global fetch API without relying on the DOM lib.

Other information

  1. The switch to the Fetch API is particularly beneficial for MQTTX CLI as it doesn't heavily rely on complex HTTP operations, making the full feature set of Axios unnecessary.
  2. This change aligns the project with modern JavaScript practices and reduces external dependencies.
  3. Reviewers should pay special attention to the TypeScript configurations and ensure that the Fetch API is correctly typed without introducing DOM-related issues.

@ysfscream ysfscream added refactor Refactor code or architecture performance Improve some performance chore Changes in build tools or dependent packages CLI MQTTX CLI labels Jul 31, 2024
@ysfscream ysfscream added this to the v1.11.0 milestone Jul 31, 2024
@ysfscream ysfscream requested a review from Red-Asuka July 31, 2024 09:00
@ysfscream ysfscream self-assigned this Jul 31, 2024
@ysfscream ysfscream added the enhancement New feature or request label Jul 31, 2024
@Red-Asuka Red-Asuka merged commit b322af2 into main Jul 31, 2024
4 checks passed
@Red-Asuka Red-Asuka deleted the ysf/cli branch July 31, 2024 09:19
@Red-Asuka
Copy link
Member

Great job! I fully support using fetch instead of axios, it’s a better choice. Looking forward to us upgrading to Node 20 soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes in build tools or dependent packages CLI MQTTX CLI enhancement New feature or request performance Improve some performance refactor Refactor code or architecture
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants