mapbox / mapbox-gl-draw

Draw tools for mapbox-gl-js
https://www.mapbox.com/mapbox-gl-js/example/mapbox-gl-draw/
ISC License
940 stars 590 forks source link

Refactor for Modernize 2024 #1249

Open varna opened 2 months ago

varna commented 2 months ago

Hello,

Tank you for Modernize 2024. I'm really glad that you adopted Vite. I think this was a great choice that could simplify the pipeline and increase community engagement.

I wanted to add few additional improvements:

Add .env example file and remove no longer needed code for access_token and simplify package.json scripts.

eslint

My VScode didn't really work out of the box after cloning repo. So I tried updating editor settings, recommendations and eslint to fix that issue. Updated mourner package and eslint to flat config. Some rules became redundant and some got renamed. There are some new default rules from eslint recommended rules that mourner uses, so I had to add some rules to avoid linting error and changing thousands of lines in project. I extremely recommend to remove these a bit later:

      'strict': 0,
      'no-else-return': 0,
      'no-promise-executor-return': 0,
      'no-await-in-loop': 0,

vite

The thing is that Vite uses esbuild for dev mode and rollup for build. So using either of those on their own is kind of redundant. That's why I removed rollup. And I also simplified package scripts.

I setup Vite to work in a library mode, so you don't need to build the lib to run a html file like debug or bench. Currently both start with npm run dev. build creates umd and esm packages, but I swapped their naming a bit to match current behaviour of Draw. I recommend removing this custom naming in favour of using ESM as default and renaming UMD to .umd.cjs (because UMD is only needed for CDN). And releasing this as v2 after beta testing.

testing and bench

You got a lot of tests 🤣

I recommend using Vitest for testing. Everyone who uses it loves it and it has amazing VSCode integration. In theory it should be enough to replace all imports from node to vitest. But in practise (after trying on one file) only 80% of tests migrate that easily. I need to do more tweaking, like replacing sinon and removing AbortSignal.

Also, moving bench to vitest might be good idea (that's why I haven't finished refactoring it yet). But I haven't done benches before, might be a good practise for me. But idk if I have time for it because my boss already shouting at me for being a commie that wastes time on OS.

feedback

So, I wanted to get opinions and feedback on my ideas and proposed changes.