Typescript: Remove @mapbox/geojson-types and replace with @types/GeoJSON'

[chippieTV]

• confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

• Remove imports of @mapbox/geojson-types

• Update GeoJSON types (untested build as TS WIP, but reduces tsc errors)

• Errors from 615 -> 609

• Marked as draft because currently difficult to test. I will try to improve the PR when I can get things building and more stable

• add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>

[lseelenbinder]

Looks good to me, aside from testing. Thanks @chippieTV!

[HarelM]

I think Object.keys(...) will be better here and or simply clone using JSON.parse(JSON.Stringify) and remove the non-required fields. not sure. probably the first one is good enough...

[chippieTV]

I left the other errors alone in the hope to make a targeted PR, (I actually removed another unrelated file change and push force to put it in a different misc PR).

So for this, the Maplibre code conventions mentioned not using certain features to keep compatibility with IE11.. Spread falls into that. Otherwise the whole thing could be reduced like this:

// BEFORE
toJSON() {
    const json = {
        geometry: this.geometry
    };

    for (const i in this) {
        if (i === '_geometry' || i === '_vectorTileFeature') continue;
        json[i] = (this as any)[i];
    }
    return json;
}

// AFTER
toJSON() {
  // some tslint/eslint ignore-next-line unused-vars hint
  const {_geometry, _vectorTileFeature, ...json} = this;
  return json;
}

TS Playground link

But in theory we can't do that right?

So I don't know how far we want to go, it's possible to set the TS compiler to output whatever we want compatibility wise, so writing up to date syntax (we're not writing JS at this point anyway) and output to ES5 might be a good option if we really want to support IE11?

(some blog post about target versions)

It's quite a broad decision so I can't make it, nor guarantee that it's the right way to go as it's quite specifically against the code conventions, however I assume so is TS..

[HarelM]

No need to support IE 11.
But the code you wrote is not equivalent to the original code :-)

[chippieTV]

😮 really?!

so weird to me.. I mean, if geometry exists on this why doesn't it get extracted into json?.. because get/set?

confirmed you're right though TS playground

[HarelM]

No, no, you added extra fields to the json that are removed as part of this method. See the continue line above... :-)

[HarelM]

In the version that uses "..." I mean...
I think Object.keys is good enough for now...

[chippieTV]

so my understanding of what I wrote is

original

1. create json object with geometry property from this.geometry
2. loop through all properties on this
3. if current property is _geometry or _vectorTileFeature, skip adding to json object
4. if current property is anything else, add to json object
5. return json object

my spread operator version is saying

1. save _geometry and _vectorTileFeature as separate consts (then throw them away - same as the if/continue bit)
2. assign everything else to json

like this

[HarelM]

You are right, I misread it :-) my bad...
I think the new version is just fine.

[chippieTV]

it's good to keep an eye on it though, hopefully the test suite can do some magic and pick up any weirdness introduced in this or any other of the changes we make converting to TS!

[HarelM]

Other than the above looks good to me.

[HarelM]

You want me to merge this? I see that the title is draft...?

[chippieTV]

I removed draft, if you're happy with it it's fine with me