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

Merged
merged 2 commits into from
Jan 30, 2025

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: 0b4383c

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 [s] Min [s] Max [s] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 1.032 ± 0.022 1.013 1.090 1.00 ± 0.03
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 1.030 ± 0.023 0.998 1.076 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 0b4383c

@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.

packages/cli/src/__tests__/commands/push.test.ts Outdated Show resolved Hide resolved
packages/cli/src/__tests__/commands/push.test.ts Outdated Show resolved Hide resolved
packages/cli/src/cms/api/api-client.ts Outdated Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/cli/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.github/workflows/smoke.yaml Show resolved Hide resolved
@DmitryAnansky DmitryAnansky force-pushed the feat/remove-node-fetch-from-core branch from e8efe7c to 2f7e4b0 Compare January 21, 2025 15:31
@DmitryAnansky DmitryAnansky requested a review from tatomyr January 21, 2025 15:32
@DmitryAnansky DmitryAnansky requested a review from tatomyr January 21, 2025 15:48
@DmitryAnansky DmitryAnansky force-pushed the feat/remove-node-fetch-from-core branch 2 times, most recently from 0e25669 to 308a13c Compare January 22, 2025 14:27
@DmitryAnansky DmitryAnansky added the snapshot Create experimental release PR label Jan 22, 2025
@tatomyr
Copy link
Contributor

tatomyr commented Jan 24, 2025

@DmitryAnansky could you also update the README with the new required versions of Node/NPM?
Also, does this resolve #1332 ? If yes, please mention that in the description.

@DmitryAnansky DmitryAnansky force-pushed the feat/remove-node-fetch-from-core branch from be8c554 to 0b4383c Compare January 24, 2025 15:07
@DmitryAnansky
Copy link
Contributor Author

@DmitryAnansky could you also update the README with the new required versions of Node/NPM? Also, does this resolve #1332 ? If yes, please mention that in the description.

ReadMe is updated now,
If this change applied, I will test if the issue fixed and update the issue.

@DmitryAnansky DmitryAnansky merged commit 233f6a8 into main Jan 30, 2025
41 checks passed
@DmitryAnansky DmitryAnansky deleted the feat/remove-node-fetch-from-core branch January 30, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snapshot Create experimental release PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants