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

feat: migrate from node-fetch to native fetch #1848

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DmitryAnansky
Copy link
Contributor

@DmitryAnansky DmitryAnansky commented Jan 17, 2025

What/Why/How?

  • bump required node version
  • migrate from node-fetch to fetch
  • fix punycode Deprecation warning message in Spot but not in CLI itself due to its inner dependencies

Reference

Testing

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Copy link

changeset-bot bot commented Jan 17, 2025

🦋 Changeset detected

Latest commit: 14b4041

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@redocly/openapi-core Minor
@redocly/cli Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 17, 2025

Command Mean [ms] Min [ms] Max [ms] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 993.7 ± 44.3 955.8 1115.5 1.02 ± 0.05
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 971.8 ± 14.3 950.6 992.7 1.00

Copy link
Contributor

github-actions bot commented Jan 17, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 78.63% 5052/6425
🟡 Branches 67.23% 2058/3061
🟡 Functions 73.16% 834/1140
🟡 Lines 78.92% 4766/6039
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡 core/src/utils.ts 78.7% 67.03% 76.47% 79.47%
🟢
... / fetch-with-timeout.ts
90.91% (-1.4% 🔻)
100% 50%
90.91% (-1.4% 🔻)
🟡
... / push.ts
73.41%
69.31% (-1.1% 🔻)
54.55% 75.76%
🟢
... / api-client.ts
82.69% 65.79% 87.5% 82.35%

Test suite run success

835 tests passing in 120 suites.

Report generated by 🧪jest coverage report action from 14b4041

@DmitryAnansky DmitryAnansky force-pushed the feat/remove-node-fetch-from-core branch from 61b597a to 4b4a862 Compare January 17, 2025 15:14
@DmitryAnansky DmitryAnansky marked this pull request as ready for review January 17, 2025 15:29
@DmitryAnansky DmitryAnansky requested review from a team as code owners January 17, 2025 15:29
@DmitryAnansky DmitryAnansky requested review from roman-sainchuk and removed request for roman-sainchuk January 17, 2025 15:49
Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

What is the remaining punycode from dependencies? Let's solve it. This warning is very annoying :(

packages/cli/src/commands/push.ts Outdated Show resolved Hide resolved
@DmitryAnansky
Copy link
Contributor Author

DmitryAnansky commented Jan 20, 2025

async function streamToBuffer

Its within dev dependencies, not sure why they got to compiled version.
Maybe they will be gone with the released version.

Screenshot 2025-01-20 at 17 35 04

(checked on 22 node)

@tatomyr
Copy link
Contributor

tatomyr commented Jan 20, 2025

@DmitryAnansky if we drop NodeJS prior to v16, we can remove the smoke tests for those here and here.

@DmitryAnansky
Copy link
Contributor Author

@DmitryAnansky if we drop NodeJS prior to v16, we can remove the smoke tests for those here and here.

sure, it is also strange that they are passing :) with the updated package.json.

@DmitryAnansky DmitryAnansky requested a review from tatomyr January 20, 2025 12:38
@DmitryAnansky DmitryAnansky force-pushed the feat/remove-node-fetch-from-core branch from 827781a to 064a85c Compare January 20, 2025 12:39
@tatomyr
Copy link
Contributor

tatomyr commented Jan 20, 2025

strange that they are passing

The smokes don't cover anything specific to fetching data, only basic operations in different environment.

@DmitryAnansky DmitryAnansky requested a review from tatomyr January 20, 2025 15:19
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.

6 participants