refactor: replace node-fetch-commonjs with native fetch API #336
Conversation
|
I have become aware that this is supported from Node 18 |
| let readBytes = 0 | ||
| res.body.on('data', (chunk: Buffer) => { | ||
| bodyStream.on('data', (chunk: Buffer) => { |
There was a problem hiding this comment.
How can we test this code? I guess a clean install would work. Did you try it?
There was a problem hiding this comment.
Do you happen to remember the local path I have to clean out to trigger a redownload? Or perhaps I can just run the command again?
There was a problem hiding this comment.
On Windows it's C:\Users\<username>\AppData\Roaming\Code\User\globalStorage\flix.flix\flix.jar
|
File is downloaded, but something else breaks |
|
It is possible this PR was broken because of #328 It is also possible its their combination that is broken. |
Strange. Neither of these PRs should touch anything that has to do with this. Are you absolutely sure it's because of these changes? |
|
Reverting boths PRs fixed the problem. I suspect the problem was not in the node fetch, but in the response types PR. Probably an issue with undefined/null etc. But I do not know. |
|
But it was 100% reproducible and revering both fixed it. |
|
I cannot reproduce it on this branch, even though it includes both of these PRs. |
|
Yes, it was very reproducible. But it seems the download completed fine and then the failure happened. Maybe its a nodejs or version issue? I switched to the newest node yesterday. How about you reopen the node-fetch PR and then I try that alone without the type stuff? |
Does the whole extension not use the version of Node built into Electron? |
|
I have no idea. I guess your point is that nodejs is only used for the build. I guess that it possible. |
You're on macOS, right? Could that make a difference? |
|
I have both Win and Mac. This was Windows actually. |
Fixes #332