search

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

First-class types for Mapbox Draw #1265

Open brookjordan opened 1 month ago

brookjordan commented 1 month ago

Now that we’ve got first-class types for MapBox GL.js, is it time we have the same for draw?

I’m currently working on types here: https://github.com/brookjordan/mapbox-gl-draw/tree/chore/convert-to-typescript/bj

I was wondering if this would be a forever-patch on my side, or if it’s something we would like to have officially supported, if I can get the first version up and running?

stepankuzmin commented 1 month ago

Hey @brookjordan,

Yeah, it's a good time to migrate GL Draw to first-class TypeScript. I first wanted to modernize the internals (i.e., switch to modern ES), but this might be easier with first-class types.

I wonder what your process for the migration is. Do you manually rewrite the JS to TS, or do you use some automated tooling? We were using patched Stripe's codemod to convert GL JS from Flow to TS.

brookjordan commented 1 month ago

Manually.

There's not a lot of code, and converting files to .ts gets you a lot of the way there, and allows me to look at what's actually going on.

Like, there's a bunch of functions pretending to be classes, so if I'm looking at all of the code anyway, it's quite easy to do the conversion to classes while I'm there. A small part of the es6 update.

However, I'm leaving some things the same, like object.assign, so it's easier to review with less syntactic changes. Converting to a class keeps the code very similar, other than the wrapper. Keeping the changes small enough that people are willing to review it felt important.

I'm planning to keep tests in .js for now, as there's no real demand for those to be converted, and it allows us to test how a non .ts codebase using the code could abuse the code without a plethora of ts-disable comments.

stepankuzmin commented 1 month ago

I see. Thanks for the initiative, @brookjordan!

It would be great to start small with the TypeScript migration. First, we can add initial TypeScript support to the repo and ensure compatibility with both JS and TS. Then, we can create separate PRs for converting files one by one. I'll be happy to help with this process.

brookjordan commented 1 month ago

That would actually be a nice idea… maybe the first commit in my branch would work as that first step, attempting to parse as TypeScript without strict mode.