Skip to content

fix: upgrade node-fetch to v3#24020

Merged
Jolg42 merged 6 commits intoprisma:mainfrom
avallete:fix/node-fetch-warning
Apr 30, 2024
Merged

fix: upgrade node-fetch to v3#24020
Jolg42 merged 6 commits intoprisma:mainfrom
avallete:fix/node-fetch-warning

Conversation

@avallete
Copy link
Copy Markdown
Contributor

@avallete avallete commented Apr 29, 2024 •
edited by janpio
Loading

What happens

  • Upgraded "node-fetch" to v3 to get rid of the punycode warning

Reason

I use @prisma/internals in a 3rd party to read and parse the content of the schema.prisma.
Issue is, importing config parser will trigger a warning for node >= 21:

node --trace-deprecation
Welcome to Node.js v21.7.3.
Type ".help" for more information.
> const d = require('@prisma/internals')
> (node:40608) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
    at node:punycode:3:9
    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:398:7)
    at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/realm:337:10)
    at loadBuiltinModule (node:internal/modules/helpers:104:7)
    at Module._load (node:internal/modules/cjs/loader:999:17)
    at Module.require (node:internal/modules/cjs/loader:1230:19)
    at require (node:internal/modules/helpers:179:18)
    at ../../node_modules/.pnpm/whatwg-url@5.0.0/node_modules/whatwg-url/lib/url-state-machine.js (<retracted>/node_modules/@prisma/internals/node_modules/@prisma/engines/dist/index.js:16630:20)
    at __require (<retracted>/node_modules/@prisma/internals/node_modules/@prisma/engines/dist/index.js:9:50)
    at ../../node_modules/.pnpm/whatwg-url@5.0.0/node_modules/whatwg-url/lib/URL-impl.js (<retracted>/node_modules/@prisma/internals/node_modules/@prisma/engines/dist/index.js:17700:15)

Based over the comment linked in this issue: #21644 (comment)

One of the way to resolve the issue mentioned is to upgrade node-fetch to version 3. That's the attempt made here.

I did my best to follow the recommendations provided by node-fetch here

Also they did mention that their new minimum supported version of node is 12.20 which is above the one for prisma (16.13).

closes #21644

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 29, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

avallete
avallete commented Apr 29, 2024
View reviewed changes
avallete
avallete commented Apr 29, 2024
View reviewed changes
@avallete avallete marked this pull request as ready for review April 29, 2024 20:26
@avallete avallete requested a review from a team as a code owner April 29, 2024 20:26
@avallete avallete requested review from Druue and removed request for a team April 29, 2024 20:26
@janpio
Copy link
Copy Markdown
Contributor

janpio commented Apr 30, 2024

(I edited your description to mention that this "closes #21644" - because that was an issue about this warning)

@Jolg42 Jolg42 self-requested a review April 30, 2024 09:35
Jolg42
Jolg42 reviewed Apr 30, 2024
View reviewed changes
@Jolg42 Jolg42 added this to the 5.14.0 milestone Apr 30, 2024
@Jolg42 Jolg42 self-assigned this Apr 30, 2024
@Jolg42
Copy link
Copy Markdown
Contributor

Jolg42 commented Apr 30, 2024

@avallete Thanks for the PR 🙌🏼

I added a nitpick comment, it's a nice contribution!

Before we can merge this, could you check this comment about signing the CLA -> #24020 (comment)

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 30, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #24020 will not alter performance

Comparing avallete:fix/node-fetch-warning (b97248e) with main (ccebff2)

Summary

✅ 3 untouched benchmarks

@avallete
Copy link
Copy Markdown
Contributor Author

avallete commented Apr 30, 2024

@avallete Thanks for the PR 🙌🏼

I added a nitpick comment, it's a nice contribution!

Before we can merge this, could you check this comment about signing the CLA -> #24020 (comment)

Hey @Jolg42 thank's for your review and comments. I've applied the suggested patch and signed the CLA 👍

I'm seing a failing test on windows CI it seems that the localhost server is unreachable for some reason, but I have to admit I'm having troubles to grasp the meaning of the error. If it's a blocker I could use some help to reproduce this as I don't own a windows machine.

@avallete avallete requested a review from Jolg42 April 30, 2024 11:35
@Jolg42
Copy link
Copy Markdown
Contributor

Jolg42 commented Apr 30, 2024

@avallete Unfortunately sometimes tests fail for no good reason.
I guess you are mentioning this error:

Rejected to value: [Error: P1001: Can't reach database server at `localhost`:`3306`·
    Please make sure your database server is running at `localhost`:`3306`.]

It's unrelated, it's caused by the Docker setup, usually a retry gets rid of it, it's ok to ignore.

@Jolg42
Copy link
Copy Markdown
Contributor

Jolg42 commented Apr 30, 2024

Actually, it looks like the MySQL version got upgraded on Windows / Scoop and it's broken since.
We did not pin the version before, so it broke in all branches today.
I opened a PR to pin the version #24030

@Jolg42 Jolg42 merged commit 438a718 into prisma:main Apr 30, 2024
@Jolg42
Copy link
Copy Markdown
Contributor

Jolg42 commented Apr 30, 2024

@avallete You can now try the dev version 5.14.0-dev.30. If you do, let us know how it goes, it's always appreciated to get some positive feedback/confirmation.

This will be part of the next official release planned for May 14th.

@avallete
Copy link
Copy Markdown
Contributor Author

avallete commented May 1, 2024

@avallete You can now try the dev version 5.14.0-dev.30. If you do, let us know how it goes, it's always appreciated to get some positive feedback/confirmation.

This will be part of the next official release planned for May 14th.

Hey @Jolg42 ! I can confirm that after upgrading to 5.14.0-dev.30 the punycode warning when using @prisma/internals are now gone.

Thank you for your help reviewing this and getting it shipped !

@adriencaccia adriencaccia mentioned this pull request May 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Druue Druue Awaiting requested review from Druue Druue was automatically assigned from prisma/orm-ts

1 more reviewer

@Jolg42 Jolg42 Jolg42 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Jolg42 Jolg42

Labels

None yet

Projects

None yet

Milestone

5.14.0

Development

Successfully merging this pull request may close these issues.

Usage of deprecated punycode module

4 participants

@avallete @CLAassistant @janpio @Jolg42