CivPlatform / map-sync

Mod to sync map data with others instantly and privately, and update tiles for Leaflet/CivMap
GNU General Public License v3.0
28 stars 10 forks source link

Remove node-fetch and its typings #67

Closed Protonull closed 1 year ago

Huskydog9988 commented 1 year ago

Can't we replace this with the native fetch api node introduced?

Gjum commented 1 year ago

Some distros don't make it easy to use a recent nodejs, but if we require a recent nodejs for other features anyway we can switch to builtin fetch. Are there any benefits from using the builtin fetch vs node-fetch 2?

Huskydog9988 commented 1 year ago

Not really sure to be honest, and personally I just use Docker so node versions don't affect me much. So I'm fine with whatever you wanna do.

Protonull commented 1 year ago

Just tested and node 18 has native fetch, so the module shouldn't be necessary anymore

Protonull commented 1 year ago

All good now, this is now a 'yeet node-fetch' PR

Huskydog9988 commented 1 year ago

I assume this is tested?

Protonull commented 1 year ago

I assume this is tested?

$ node --version
v18.17.1
// test.js
fetch("https://github.com/CivPlatform/map-sync/pull/67")
    .then((res) => res.text())
    .then((txt) => console.log("SUCCESS!"))
    .catch((err) => console.error("Err!", err))
$ node test.js
SUCCESS!
Huskydog9988 commented 1 year ago

Good enough for me!