search

maplibre / maplibre-gl-geocoder

Geocoding for MapLibre
ISC License
57 stars 19 forks source link

Update CI #47

Closed wipfli closed 2 years ago

wipfli commented 2 years ago

Updates the CI files to node version 16. Introduces a test workflow. Removes legacy travis config file.

Cactusbone commented 2 years ago

Looks okay to me. However running against node 16 instead of node 10 might hide some problems, when the package.json only ask for node > 6.

Seeing as package-lock is already using version 2, I think updating node requirement in package.json as well would be nice :)

Why did you update to node 16 here ? How about adding test workflow using node 10 here and updating in another PR ?

wipfli commented 2 years ago

Good question. I guess you are talking about this section:

https://github.com/maplibre/maplibre-gl-geocoder/blob/b0a3db7d52fdac1c03cbbdbc65ba437005bad5c9/package.json#L25-L27

IIRC, this is only used by node but since this plugin runs in a browser, we can remove it?

wipfli commented 2 years ago

I think there was some discussion about the node engine in GL JS...

wipfli commented 2 years ago

@HarelM

Cactusbone commented 2 years ago

IIRC, this is only used by node but since this plugin runs in a browser, we can remove it?

Actually, I'm using it as a npm dependency and it's packaged using browserify (like prepublish step).

Keeping it allows some tools (like nodist on windows) to pick an appropriate node to run commands. However this package does not seems to have any :)

Looks okay to me to update to node 16 to build and test. You may lose node 6 compatibility without noticing, but that was already the case since you were test using node 12. (ie if you start using const/let instead of var). This should be handled by browserify though...