Skip to content
This repository has been archived by the owner on Mar 21, 2023. It is now read-only.

chore: update node-fetch #405

Closed
wants to merge 1 commit into from
Closed

Conversation

kpodsiad
Copy link
Member

There is one change in the source code.

"@types/node-fetch" has been deprecated - This is a stub types definition. node-fetch provides its own type definitions, so you do not need this installed.

@kpodsiad
Copy link
Member Author

I wanted to write some tests but they're failing because jest cannot consume the newest node-fetch:

node-fetch was converted to be a ESM only package in version 3.0.0-beta.10. node-fetch is an ESM-only module - you are not able to import it with require.

@gabro is this an issue? Will this PR and ESM only thing affect metals-vscode and metals-coc? I'm not an expert in commonjs/esm modules stuff.

@gabro
Copy link
Member

gabro commented Jan 21, 2022

It should be ok since typescript should take care of it, but in case it generates issues we can change the build target to esm as well.

Unfortunately I'm not uber-expert of ESM neither, so I recommend trying locally that the applications using this library still work.

@ckipp01
Copy link
Member

ckipp01 commented Jan 21, 2022

Also heads up that coc-metals has been lagging behind for a while, mainly because I just haven't been updating it or really working on it anymore due to nvim-metals. So don't let that hinder this at all or I wouldn't worry too much about ensuring it works with coc-metals

@gabro gabro removed their request for review February 11, 2022 08:25
@tanishiking
Copy link
Member

tanishiking commented Mar 22, 2022

I did some study about ESM, we have three choices. My conclusion: stay v2

1) Wait for VSCode (Electron) to support ESM

2) Wait TypeScript to stabilize "module": "node12" (and dynamic loading pure ESM package (I mean, node-fetch))

3) Stay in node-fetch v2 until 1 or 2 is ready (My recommendation for now)

As node-fetch says

You can also stay with the v2 branch - We will keep updating v2 with bug/security issues. But not so much with new features...
node-fetch/node-fetch#1279 (comment)

There's no rush to upgrade node-fetch to v3, just chill and wait for the upstream improvements :)

tanishiking added a commit to tanishiking/metals-languageclient that referenced this pull request Mar 23, 2022
@tanishiking
Copy link
Member

Opened a PR dependabot not to upgrade node-fetch to v3 #434

@kpodsiad
Copy link
Member Author

Thanks for the analysis @tanishiking, I was never brave enough to do such thing and deep dive into JS modules worlds :D

I think we can close this PR.

@kpodsiad kpodsiad closed this Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants