inline types into d.ts

[rotu]

After this PR, the released maplibre-gl.d.ts should be self-contained, requiring no additional external libraries to resolve types.

1. Use a json file to store config. Especially adding so many extra command line options, this makes the build process a bit more straightforward.
2. The previous d.ts has a bunch of imports. This means you needed to install those type packages to properly consume the exported types. This should no longer be the case.:

import Point from '@mapbox/point-geometry';
import TinySDF from '@mapbox/tiny-sdf';
import { VectorTileFeature, VectorTileLayer } from '@mapbox/vector-tile';
import { mat2, mat4, vec4 } from 'gl-matrix';
import { PotpackBox } from 'potpack';

Note that there are some conflicts between declarations of types named Point and Feature. I'm not quite sure how to resolve them. These don't break the build but do produce some warnings that will probably show up in the IDE as well:

dist/maplibre-gl.d.ts(1106,15): error TS2395: Individual declarations in merged declaration 'Point' must be all exported or all local.
dist/maplibre-gl.d.ts(1595,13): error TS2300: Duplicate identifier 'Feature'.
dist/maplibre-gl.d.ts(3149,36): error TS2339: Property 'type' does not exist on type 'GeoJSON'.
dist/maplibre-gl.d.ts(3214,18): error TS2395: Individual declarations in merged declaration 'Point' must be all exported or all local.
dist/maplibre-gl.d.ts(3273,18): error TS2300: Duplicate identifier 'Feature'.
dist/maplibre-gl.d.ts(3295,18): error TS2315: Type 'Feature' is not generic.

Error: Compiled with errors

[rotu]

Inlining the types by package might be really dumb - it might make sense to add a d.ts file to this project to re-export only the external types that we need; or even to remove external dependencies on @types/mapbox__point-geometry, et al, preferring to maintain those types locally.

[HarelM]

Point geometry is used by vector tile, this is one of the reasons for keeping it outside this library.
I've opened a PR for these packages to include the types in them and not in definitely typed, but it didn't work...
So it's a mess...
Can you include the generated d.ts file here and I'll test it in one of my projects?

[rotu]

Point geometry is used by vector tile, this is one of the reasons for keeping it outside this library. I've opened a PR for these packages to include the types in them and not in definitely typed, but it didn't work... So it's a mess...

I still don't quite get it ("used by vector tile" = this type must be kept in sync with X function in Y package). Even if the classes are implemented in external packages, we can still maintain pure d.ts typings here instead of definitely typed.

Anyway, one of the issues with inlining the types is the real naming collisions. E.g. geojson has a Point type and so does @mapbox/point-geometry. Whether we inline the types or expect the user to import them from external libraries, the names still clash.

Can you include the generated d.ts file here and I'll test it in one of my projects?

Here's the dist with the generated types:
dist.zip

[rotu]

BTW, we also might consider using rollup-plugin-dts or @microsoft/api-extractor to generate the typings instead. I don't know if they handle externally declared types any better.

[HarelM]

Well... It's a long story...
The best solution I found back when I tried to hack this was the current solution.
We started with moving the code of point geometry into this library, but mapbox/vector-tile is using it. VT is a large package I don't think we should add here, it's too complicated.
The other problem is the two step build process which is problematic for typescript extraction.
We need to export this package as umd and this also creates a problem which, back then only dts-bundle-generator solved.
Bottom line, it's not a easy fix. I remember trying to get the api generator to work and it didn't create a valid or similar ts api (similar to mapbox-gl that was generated manually in definitely typed) but this was when we moved to version 2 and typescript, it might have changed since then, feel free to explore, but I would recommend to have a real project you can test this with.

Colliding classes definitions is not ideal.
Also note that the current solution is not perfect, there is some patches applied after the generation is completed to solve some public classes issues...

[HarelM]

Also the build is failing, so I'm not sure this solution is an easy fix... :-(
@rotu can we merge #2014, release a pre-3 and then decide how to push this forward.
I generally think there should be a better solution to this issue than the current implementation, the current implementation is hacky at best, but this doesn't look like an easy fix and I would like to create a pre-release that fixed the problem in pre.2 and then have all the time we need to properly solve this, what do you say?

[HarelM]

current implementation = what's in the main branch, not this what's in this PR. Generally speaking, this PR looks in the right direction, or using a different tool might do the work, IDK...

[HarelM]

@rotu where are we on this?

[rotu]

Blocked by #2054

And I'm not sure how to move forward with the Point and Feature types mentioned above.

[HarelM]

If anything, I would consider migrating to the rollup plugin as it is more aligned with how we build this library, I'm not sure how well it will work though.
In any case, the blocking PR was merged.
Please let me know if you would like to push this forward or close this PR.
Thanks!

[rotu]

If anything, I would consider migrating to the rollup plugin as it is more aligned with how we build this library, I'm not sure how well it will work though. In any case, the blocking PR was merged. Please let me know if you would like to push this forward or close this PR. Thanks!

I'll give this another look.

I admire Rollup but I also feel like it's unwieldy and has led to some iffy choices. I actually feel like we should be reducing the number of rollup plugins (e.g. rollup-plugin-sourcemaps, @rollup/plugin-replace, @rollup/plugin-strip) rather than adding new ones.

Here are the extant issues:

dist/maplibre-gl.d.ts(3,38): error TS2440: Import declaration conflicts with local declaration of 'Feature'.
dist/maplibre-gl.d.ts(146,15): error TS2395: Individual declarations in merged declaration 'Point' must be all exported or all local.
dist/maplibre-gl.d.ts(3802,36): error TS2339: Property 'type' does not exist on type 'GeoJSON'.
dist/maplibre-gl.d.ts(3867,18): error TS2395: Individual declarations in merged declaration 'Point' must be all exported or all local.
dist/maplibre-gl.d.ts(3948,18): error TS2315: Type 'Feature' is not generic.

[HarelM]

@rotu do you want to push this forward or close this PR?
I'm trying to clean up the PR list a bit...

[rotu]

Updated. All the issues are still present that I mentioned in #2015 (comment) and these prevent further movement of this PR.

[HarelM]

I'm not sure I fully understand what's blocking you, can you please elaborate?

[codecov-commenter]

Codecov Report

Patch and project coverage have no change.

Comparison is base (f93a377) 73.83% compared to head (5b260d2) 73.83%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2015   +/-   ##
=======================================
  Coverage   73.83%   73.83%           
=======================================
  Files         238      238           
  Lines       18961    18961           
  Branches     4277     4277           
=======================================
  Hits        14000    14000           
  Misses       4961     4961           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rotu]

I'm not sure I fully understand what's blocking you, can you please elaborate?

Sorry - I can see how that could be very unclear!

Feature has three conflicting definitions. One in maplibre-gl-js, one in @maplibre/maplibre-style-spec, and one from GeoJSON:

maplibre-gl-js/src/source/geojson_wrapper.ts

Lines 8 to 20 in 1cbb496

	// The feature type used by geojson-vt and supercluster. Should be extracted to
	// global type and used in module definitions for those two modules.
	export type Feature = {
	type: 1;
	id: any;
	tags: {[_: string]: string | number | boolean};
	geometry: Array<[number, number]>;
	} | {
	type: 2 | 3;
	id: any;
	tags: {[_: string]: string | number | boolean};
	geometry: Array<Array<[number, number]>>;
	};

https://github.com/maplibre/maplibre-style-spec/blob/5c4b8b62f01ed4bea4fa4c6f0853c6bd58aa3d71/src/expression/index.ts#L33-L45

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/4e99c4c87d11cf464312b6150a1e5001be4f809b/types/geojson/index.d.ts#L147-L166

I'm not sure if the other issues are still true blockers but the Feature type definitely is.

[HarelM]

Well yes... Typescript lacks the concept of namespace so this is expected I guess...
I don't mean to be blunt, but I find the motivation around this effort weak at best as I'm not sure what this will actually solves for the end customer.
Having said that, if this is something you want to solve go ahead 😀

[rotu]

Well yes... Typescript lacks the concept of namespace so this is expected I guess... I don't mean to be blunt, but I find the motivation around this effort weak at best as I'm not sure what this will actually solves for the end customer. Having said that, if this is something you want to solve go ahead 😀

The bluntness is appreciate! And yes, I agree this is fairly navel-gazy.

I do think it’s important to fix the confusion around multiple “Feature” types. Do you have any insight why they all exist?

[HarelM]

Geojson feature, vector tile layer feature, and probably another one...
It's a common word 😄

[HarelM]

I'm closing this for now, feel free to reopen if needed.