Improve exports

[HarelM]

Launch Checklist

This is also part of version 4 breaking changes in order to solve the mess we have with the typescript exports.

• Breaking changes MapLibre GL JS v4 #3052

This mainly removes the getters and setters and the global default exported static object in favor of functions and named exports.

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[codecov-commenter]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (fa7372f) 83.28% compared to head (01088cc) 74.89%.

Files	Patch %	Lines
src/index.ts	89.33%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3601      +/-   ##
==========================================
- Coverage   83.28%   74.89%   -8.40%     
==========================================
  Files         244      242       -2     
  Lines       32223    19229   -12994     
  Branches     4398     4257     -141     
==========================================
- Hits        26837    14401   -12436     
+ Misses       5138     4828     -310     
+ Partials      248        0     -248     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Ok, seems like I fixes the tests, which is good.
I still need to fix the docs now, which I'm not sure what broke them, this change or a previous upgrade of the library, I'll check.
@timocov still waiting for you input regarding standalone.ts, and in general, if this will resolve the typescript issues I've faced in the past.

[timocov]

@HarelM do you have understand/stats on what your users prefer to use more - "standalone" version or csp one? IMO "standalone" version is helpful for tests and playgrounds, but esm version (or as you call it "csp") is a way to go in most of the cases. Reasons as I see it:

• "csp" is more secured by default, which is much better to everyone and provides best practices out of the box
• with "csp" your customers can control where to upload worker module
• it might improve overall performance of the library loading as the main bundle is smaller (it doesn't contain worker's code) and in theory with some hints the browser can be preload worker's code in parallel to the main bundle (yeah worker's code might contain another version of "shared" code but I think it should be fine and the price isn't that big)
• it simplifies your support for public API significantly (you don't need to worry about 2 ways of importing the library and maintain them)
• the cost for your clients might not be that huge - all they need to do is to use one of the loaders in their build system to "copy" file to the deployment folder and provide you a link to that worker module. In reality in most build systems (webpack, rollup, I'm sure you can find something similar in other as well) it is done literally with 1 line of code (e.g. use file-loader with the reference to the worker's chunk and then provide its result to setWorkersUrl() function). This can be covered in the docs for top 3-5 bundlers.

I'm not saying removing "standalone" version completely (I'm sure you might find it useful it tests or a playground), but at least change the defaults and do not let people misuse it (e.g. by playing with exports in package.json).

[HarelM]

I think most people are using the standalone version, it's a lot easier to set up and most of the wrapping libraries I saw do not contain the docs to allow using the csp version. I don't have any statistics, but I assume most people use the standalone version, it's a lot more simple...

[HarelM]

After seeing what you did here, simple for you might be very complicated for normal developers 🤪

[timocov]

Yeah that makes sense. I'd still recommend at least to change the defaults (csp by default) but leave an ability to load a standalone version via different import path. The fact that some people don't read the docs should affect the design of the library 😂 they might use the standalone version because it's the default now... but yeah, it's totally up to you.

In this case I think you might need to bring the umd thing back as I assumed (wrongly) that a standalone version isn't used to import the library as a module (option 3 in your examples), only to "add it to the bundle" via import and then rely on the "global" object that was created as a result of a side-effect of that import. Apologies

[HarelM]

The render tests and the examples are using the global object, so the original option 1 is OK, there's no tests for option 3, I should probably add one somehow...

[HarelM]

Seems like the docs broke in pre.4 release and not by this PR.
I need to consider if this is read to be merged or not (besides the changelog that I still need to do).
The changes here are not that big, which is good.

[HarelM]

Still a few TODOs I need to solve before I can merge this.
One is the debug stuff that I commented out, and the controller interface that is related to the docs that I need to find a solution to.

[HarelM]

Docs are still broken though, I need to think what to do with them...

[timocov]

@HarelM is it related to the generated bundle declaration file?

[HarelM]

I don't think so, but it uses the entry point and exported stuff to generate the API docs, which now seems a bit different, due to the changes, but also missing some grouping which I'm not sure how to solve...

[HarelM]

This is generally ready for review.
There's a few things I added here, which I also added in #3611, so they will be removed once the other PR is merged.
I still need to think what to do about the debug part the I have removed...

[HarelM]

I have removed the duplicate stuff that were here and in the docs fix PR.
@timocov what do you think is the best option to solve the following warnings:

The following type nodes were renamed because of the name collisions and will not be exported from the generated bundle:
- FullscreenOptions (from /Users/harelmazor/dev/maplibre-gl-js/src/ui/control/fullscreen_control.d.ts)
This might lead to unpredictable and unexpected output, and possible breaking changes to your API.
Consider either (re-)exporting them explicitly from the entry point, or disable --export-referenced-types option ('output.exportReferencedTypes' in the config).

[timocov]

It really depends on whether you want to your consumers use import and use FullscreenOptions type. If they should, then export this type explicitly, otherwise you can disable --export-referenced-types, but be aware that most likely it will remove from exporting some other types so it is worth checking the diff in this case and export what you actually need (but I assume this is not your route).

[HarelM]

Well, the type is part of a constructor argument, the FullScreenControl class, there's no real value in exporting it on one hand, but it is relevant to the user who instantiate FullScreenControl.
I can probably rename it FullScreenControlOptions to avoid collision, but then I'll probably need to rename all the others.
IDK... the concept of explicit adding types to the API only to avoid collision seems a bit forced. I understand the concept around explicitly exporting things, but sometimes it feels odd.
I guess I'll rename it, probably the easiest solution...

[neodescis]

Well, the type is part of a constructor argument, the FullScreenControl class, there's no real value in exporting it on one hand, but it is relevant to the user who instantiate FullScreenControl. I can probably rename it FullScreenControlOptions to avoid collision, but then I'll probably need to rename all the others. IDK... the concept of explicit adding types to the API only to avoid collision seems a bit forced. I understand the concept around explicitly exporting things, but sometimes it feels odd. I guess I'll rename it, probably the easiest solution...

FWIW, I find the exported constructor argument types to be quite important. The options are not always defined in the same place that the controls are constructed.

[HarelM]

The type is exported in both the docs and the d.ts file.
The problem here is that I need to explicitly say that I need to export it as part of index.ts, which for me, seems like a maintenance overhead.
I see this in both dts-bundle-generator and in the typedoc.
While I understand that the public API is important, re-exporting everything "manually" is error prone and verbose.
I must be missing on something basic here.
My last commit adds a lot of classes and interfaces to the public API surface, mainly, from my perspective, to comply with typedoc document generation.

[timocov]

While I understand that the public API is important, re-exporting everything "manually" is error prone and verbose.

@HarelM That's exactly why --export-referenced-types exists, but sometimes the tool wants to perform safe operations while providing better DX - the name collisions resolver mechanism was added for sake of better DX (you don't need to name objects with globally unique names anymore) but the cost is that if you want to use "please export everything for me" feature, you need to do at least something. You don't expect tools read your mind, do you? 😄

In v10 it would be even more error-prone - the tool will fail instead of just warnings. At least it can prevent you from accidental "hiding" of a type (this can happen right now if you don't monitor warnings). The only advice I can give - try to avoid using names that are available globally in DOM or NodeJS API (FullscreenOptions, Map, etc are available and it might lead to mistakes).

My last commit adds a lot of classes and interfaces to the public API surface, mainly, from my perspective, to comply with typedoc document generation.

Are you running typedoc on the generated bundle dts file or index.ts as is? Unless you want to reference source files where a type is located, it might be better to run it on the bundle so you don't need to comply with 2 tools but just one 😄

[HarelM]

I'm running it on the index.ts file.
Not sure I mind the location of the source file much, I'll experiment with the docs to see how it looks if I run it on the d.ts, but I'm not sure it will be a simple change :-)

[HarelM]

Well, I get the following warnings when I run typedoc on the d.ts file:
So the tools are doing similar things but differently... :-/

[warning] GeoJSONFeature, defined in ./dist/maplibre-gl.d.ts, is referenced by MapGeoJSONFeature but not included in the documentation.
[warning] Map$1, defined in ./dist/maplibre-gl.d.ts, is referenced by MapMouseEvent.target but not included in the documentation.
[warning] StyleLayer, defined in ./dist/maplibre-gl.d.ts, is referenced by Style.getLayer.getLayer but not included in the documentation.
[warning] OverscaledTileID, defined in ./dist/maplibre-gl.d.ts, is referenced by GetGlyphsParamerters.__type.tileID but not included in the documentation.
[warning] Event$1, defined in ./dist/maplibre-gl.d.ts, is referenced by HandlerResult.__type.originalEvent but not included in the documentation.
[warning] DEMData, defined in ./dist/maplibre-gl.d.ts, is referenced by RequestResponseMessageMap.__type.loadDEMTile but not included in the documentation.
[warning] AlphaImage, defined in ./dist/maplibre-gl.d.ts, is referenced by StyleGlyph.__type.bitmap but not included in the documentation.
[warning] RGBAImage, defined in ./dist/maplibre-gl.d.ts, is referenced by StyleImageData.__type.data but not included in the documentation.
[warning] FeatureIndex, defined in ./dist/maplibre-gl.d.ts, is referenced by WorkerTileResult.__type.featureIndex but not included in the documentation.
[warning] ImageAtlas, defined in ./dist/maplibre-gl.d.ts, is referenced by WorkerTileResult.__type.imageAtlas but not included in the documentation.
[warning] Tile, defined in ./dist/maplibre-gl.d.ts, is referenced by Source.abortTile.abortTile.tile but not included in the documentation.
[warning] Dispatcher, defined in ./dist/maplibre-gl.d.ts, is referenced by SourceClass.__type.new SourceClass.dispatcher but not included in the documentation.
[warning] Failed to resolve link to "Map#updateImage" in comment for StyleImageInterface.render.
[warning] Failed to resolve link to "Map#updateImage" in comment for StyleImageInterface.render.__type.
[warning] Failed to resolve link to "Map#addImage" in comment for StyleImageInterface.onAdd.
[warning] Failed to resolve link to "Map#addImage" in comment for StyleImageInterface.onAdd.__type.
[warning] Failed to resolve link to "Map#removeImage" in comment for StyleImageInterface.onRemove.
[warning] Failed to resolve link to "Map#removeImage" in comment for StyleImageInterface.onRemove.__type.
[warning] Failed to resolve link to "Map#fitBounds" in comment for PaddingOptions.
[warning] Failed to resolve link to "Map#fitScreenCoordinates" in comment for PaddingOptions.
[warning] Failed to resolve link to "Map#setPadding" in comment for PaddingOptions.
[warning] Failed to resolve link to "Map#addSource" in comment for addSourceType.
[warning] Failed to resolve link to "Map#addLayer" in comment for CustomLayerInterface.
[warning] Failed to resolve link to "Map#triggerRepaint" in comment for CustomLayerInterface.
[warning] Failed to resolve link to "Map#addLayer" in comment for CustomLayerInterface.onAdd.
[warning] Failed to resolve link to "Map#removeLayer" in comment for CustomLayerInterface.onRemove.
[warning] Failed to resolve link to "Map#querySourceFeatures" in comment for QuerySourceFeatureOptions.
[warning] Failed to resolve link to "Map#setStyle" in comment for TransformStyleFunction.
[warning] Failed to resolve link to "Map#jumpTo" in comment for CameraOptions.
[warning] Failed to resolve link to "Map#easeTo" in comment for CameraOptions.
[warning] Failed to resolve link to "Map#flyTo" in comment for CameraOptions.
[warning] Failed to resolve link to "Map#jumpTo" in comment for JumpToOptions.
[warning] Failed to resolve link to "Map#cameraForBounds" in comment for CameraForBoundsOptions.
[warning] Failed to resolve link to "Map#flyTo" in comment for FlyToOptions.
[warning] Failed to resolve link to "Map#easeTo" in comment for FlyToOptions.__type.curve.
[warning] Failed to resolve link to "Map#fitBounds" in comment for FitBoundsOptions.
[warning] Failed to resolve link to "Map#easeTo" in comment for FitBoundsOptions.__type.linear.
[warning] Failed to resolve link to "Map#flyTo" in comment for FitBoundsOptions.__type.linear.
[warning] Failed to resolve link to "Map#panBy" in comment for AnimationOptions.
[warning] Failed to resolve link to "Map#easeTo" in comment for AnimationOptions.
[warning] Failed to resolve link to "Map#on" in comment for MapLayerEventType.
[warning] Failed to resolve link to "Map#on" in comment for MapEventType.
[warning] Failed to resolve link to "Map#remove" in comment for MapEventType.__type.remove.
[warning] Failed to resolve link to "Map#addImage" in comment for MapEventType.__type.styleimagemissing.
[warning] Failed to resolve link to "Map#jumpTo" in comment for MapEventType.__type.movestart.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.move.
[warning] Failed to resolve link to "Map#jumpTo" in comment for MapEventType.__type.moveend.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.zoomstart.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.zoom.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.zoomend.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.pitchstart.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.pitch.
[warning] Failed to resolve link to "Map#flyTo" in comment for MapEventType.__type.pitchend.
[warning] Failed to resolve link to "Map#addControl" in comment for IControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for IControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for IControl.getDefaultPosition.
[warning] Failed to resolve link to "Map#addControl" in comment for IControl.getDefaultPosition.__type.
[warning] Failed to resolve link to "Map#addControl" in comment for NavigationControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for NavigationControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for GeolocateControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for GeolocateControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for AttributionControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for AttributionControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for LogoControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for LogoControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for ScaleControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for ScaleControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for FullscreenControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for FullscreenControl.onRemove.
[warning] Failed to resolve link to "Map#addControl" in comment for TerrainControl.onAdd.
[warning] Failed to resolve link to "Map#removeControl" in comment for TerrainControl.onRemove.

[HarelM]

Some of the problems here are due to the rename of Map to Map$1. Is there a way to tell dts-bundle-generator "I know what I'm doing, don't rename my classes"?