Make maplibre-gl-js more futureproof #291

HarelM · 2024-01-17T09:56:06Z

HarelM
Jan 17, 2024

This is in continue to several discussions in issues in this project, mainly:

Gloabl Map collides with our Map object, I think... #287

First and foremost, thank you for all the hard work put into this project, I don't want to sound ungrateful in any way, and if I come of in that way, please forgive me.

I'll see if I can describe the problem the way I see it.

The following is our public API generally:
https://github.com/maplibre/maplibre-gl-js/blob/bb219f0cc9046032b71fddc9fc59d10471bf97e3/src/index.ts

It's a bit long, let's look at how this API is begin used:

Option 1

The class is exported as part of the global window object and one can simply import the script and start using.
Our examples pages show this well:
https://maplibre.org/maplibre-gl-js/docs/examples/simple-map/

<!DOCTYPE html>
<html lang="en">
<head>
    <link rel='stylesheet' href='https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.css' />
    <script src='https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js'></script>
</head>
<body>
<div id="map"></div>
<script>
    const map = new maplibregl.Map({
        container: 'map', // container id
        style: 'https://demotiles.maplibre.org/style.json', // style URL
        center: [0, 0], // starting position [lng, lat]
        zoom: 1 // starting zoom
    });
</script>
</body>
</html>

Option 2

Importing a global variable and using it, similar to the above example, but a bit more modern:

import maplibregl from 'maplibre-gl`
const map = new maplibregl.Map({
        container: 'map', // container id
        style: 'https://demotiles.maplibre.org/style.json', // style URL
        center: [0, 0], // starting position [lng, lat]
        zoom: 1 // starting zoom
});

Option 3

The most modern one, import only what you need:

import {Map} from 'maplibre-gl`
const map = new Map({
        container: 'map', // container id
        style: 'https://demotiles.maplibre.org/style.json', // style URL
        center: [0, 0], // starting position [lng, lat]
        zoom: 1 // starting zoom
});

Another part worth mentioning are the global configurations, these are only available on the global object, for example the following code:

import maplibregl from 'maplibre-gl`
maplibregl.workerUrl = "http://www.example.com/worker-url.js"

This is the gist of it, generally, and I simply want the generated d.ts file to allow that (maybe besides the first option) these kind of usages.

I'm not sure I know how to change the public API to be different (modern?) and still facilitate for all this functionality.

Putting typescript aside, all this is working and I basically want dts-bundle-generator to understand what is being exported according to the index.ts file.

Side note: the bundling is complicated as we are using worker code and we are bundling the initialization of this worker code as part of this library, which might affect how index.ts is built and defined, and there's not a lot of room to play here.

I hope this gives a better picture, let me know if any of it is not clear.

timocov · 2024-01-18T23:29:58Z

timocov
Jan 18, 2024
Maintainer

Hi @HarelM,

Thanks for the provided information.

The use cases you shared I find similar to https://github.com/tradingview/lightweight-charts (the library I've maintained few years ago). Let me explain in more details.

How it was done

The intention of the usage of a global object was to use it in a playground, for people who'd want to use it in html files and don't use bundlers (I don't believe nowadays there is a single user of this in production tho) and it was very handy to use in integration tests/debugging purposes (e.g. see https://github.com/tradingview/lightweight-charts/blob/5d83c02416f6ccd1b8b16cecdffc6f8da183255d/debug.html.example#L9)
The exposed API was purely esm-like (without default exports, without global objects, you have to explicitly import what you need and no pollution of a global object) - similar to your 3rd option (e.g. https://tradingview.github.io/lightweight-charts/docs#creating-a-chart).

The way it was implemented is the following.

First thing - there is only 1 "real" entry point that actually exports/exposes everything that the library should expose. It is src/index.ts file. Important notes on this:

it exports only "valued" items that your library exports ("valued" in terms of typescript is something that can and will produce an output in a javascript file, e.g. classes, functions, any kind of variables and enums (including const enums in certain circumstances))
all "non-valued" items are "exported" automatically because of enabled --export-referenced-types option which automatically adds all referenced types to the generated output and marks them as "exported" (more on that below). It is handy because you don't need to declare every single export, you just mention "crucial" ones and the rest of them are exported exported automatically for you. But it is not necessary and you can disable this option and export everything you need explicitly (in fact, this option is planned to get disabled by default in v10 as a breaking change so specifying it explicitly would be a good move for having less issues in the future)
only this file is used to generate a declaration bundle

Next, the library provides "a standalone" bundle (next to the esm one - you can check the output in "development" versions if you're interested) that has all dependencies bundled in and allows you to use it in playgrounds without any issues. This bundle is created based on a file src/standalone.ts. Everything this file does is imports everything from src/index.ts file (that exports everything what the library exports), wraps it into a namespace via import * as ns syntax and assigns it into LightweightCharts global object (check the source code, it is literally 2 lines of code). This allows to achieve one important thing - everything in the global object is exactly the same as what is exported from "esm" version, there is no need to maintain this file ever again as it just "remaps" output from one file into a global object. Inlining dependencies into this bundle is done via the rollup configuration. This bundle is not supposed to be consumed by anyone in production, but it is super helpful while debugging/integration tests/playground on external resources (not library's website where it is done differently).

That's it basically. So mapping to your options:

option 1 can be done by using "standalone" bundle, see https://github.com/tradingview/lightweight-charts/blob/5d83c02416f6ccd1b8b16cecdffc6f8da183255d/README.md#cdn
option 2 is not really applicable, but from user's side it can be done via import * as LightweightCharts from 'lightweight-charts' import (which wraps all exported items from the library into an object)
option 3 is the default available option while using the library and is in mind while designing the API in the first place

This looks clean for me, your library exports functions it needs, a user has control over what they want to have in their code.

But unlike your library, lightweight-charts doesn't have any "global" state that you can configure via exposed functions. Everything belongs to a created object and you control what you have via this object. Personally, I don't see any issues with exposing functions like setWorkerUrl:

if you're afraid that the name is too ambiguous (which I can see as a potential problem), they can easily solve this issue in their code by using renaming while importing: import { setWorkerUrl as maplibreglSetWorkerUrl }. At the end of the day, you can't provide 100% name-safe API that will satisfy everyone (it doesn't mean that you shouldn't try or provide best possible).
Alternatively they can use import * as maplibregl syntax and keep it clean/safe.

Alternatively, you can wrap all global functions into a namespace/object and export it with a name like globals, maplibreGlobals or so. But I don't feel this looks good imo. I wouldn't change it.

The proposal

As for the proposal I'd suggest to make the following changes:

Remove class MapLibreGL and export everything you have there explicitly from the source file e.g. export { Map, Popup } and so on for other stuff in there
Get rid of the default export - just export everything explicitly and named
I'm not that familiar with your build process, but if you need to provide a "global version", you can get the output from the main bundle(s) and assign them to the global object with the name you need (it feels like you're doing this already, but I didn't dig into this enough).
Personally, functions/setters like workerUrl and addProtocol I'd leave as is (probably would replace setters/getters with just functions instead) as I don't see that much issues with name collision as it easily can be in user's code as I mentioned above. Having a wrapper object for all of them (e.g. globals or so) makes it look a bit weird - it would like like when you do maplibregl.globals.setWorkerUrl but if you import them directly as globals it might look odd. But again, the name collision can be solved in the consumers code easily.

I think that's it. Hopefully I didn't miss anything 😂 Let me know what you think about it and let me know if you see any issues/problems or would like me to clarify anything.

Some notes on `--export-referenced-types` flag

When you generate a dts bundle, it must represent exactly what you can see in an associated javascript file. Types/interfaces aren't presented in JS output so it can be safe to just export them all so users can use them in their code to declare variables/inputs/outputs/etc. But every use case is different and it is up to you to decide. Also it is possible that a type can be renamed due to the name collision and in this case it will not be exported automatically unless it is exported explicitly. In the current v9 you'll get a warning about it, the following v10 will fail the bundling. For more information see #286 (comment).

0 replies

HarelM · 2024-01-20T21:10:57Z

HarelM
Jan 20, 2024
Author

Thanks for the detailed answer!
The referenced library does seem similar in API and I get the resemblance.
I started trying to implement your suggestions, but due to the complexity of the second stage of build this doesn't work well.
The extra complexity is due to the need to create a small bundle size while still rigging a web worker, which is done as follows:

Create a shared chunk between worker and main thread (shared code).
Create a chuck for the main thread.
Create a chunk for the worker thread.
Set a worker url that holds the worker code and set it to the main thread.

This extra complexity (which I'm not sure I fully understand I must admit) is expecting each chunk (that is represented by an entry point) to export a default variable.
See the worker entry point:
https://github.com/maplibre/maplibre-gl-js/blob/7decb26cd6895ad595c19bd2cb6777769996c17f/src/source/worker.ts
Main entry point:
https://github.com/maplibre/maplibre-gl-js/blob/7decb26cd6895ad595c19bd2cb6777769996c17f/src/index.ts

The code that gules everything:
https://github.com/maplibre/maplibre-gl-js/blob/7decb26cd6895ad595c19bd2cb6777769996c17f/build/rollup/bundle_prelude.js
Which is related to this part:
https://github.com/maplibre/maplibre-gl-js/blob/7decb26cd6895ad595c19bd2cb6777769996c17f/build/rollup/maplibregl.js

Which is all part of the rollup complicated config:
https://github.com/maplibre/maplibre-gl-js/blob/7decb26cd6895ad595c19bd2cb6777769996c17f/rollup.config.ts

When removing the default export in index.ts, this start to break, and I'm not sure I fully understand how to fix it...

I think the main difference here between maplibre-gl and lightweight-charts is the worker code complexity (which is also the reason tree shaking is probably not possible, but this is besides the point).

Any help would be appreciated.
I can push my initial changes to a branch if that will help, I mainly changed index.ts by removing the default export and exporting everything as named exports, and switched the getters and setters to be functions.

5 replies

timocov Jan 20, 2024
Maintainer

The extra complexity is due to the need to create a small bundle size while still rigging a web worker, which is done as follows:

It feels like all these 4 steps are done by rollup right? The only chunk that should have a declaration bundle is the main thread, is that correct? I mean, a consumer doesn't use your worker file directly (i.e. import it and use some of exports from it), it should provide an URL to it (because it depends on their deployment mechanism)?

This extra complexity (which I'm not sure I fully understand I must admit) is expecting each chunk (that is represented by an entry point) to export a default variable.

Where does this requirement come from?

When removing the default export in index.ts, this start to break, and I'm not sure I fully understand how to fix it...

What errors are you getting? How can I reproduce it?

I think the main difference here between maplibre-gl and lightweight-charts is the worker code complexity (which is also the reason tree shaking is probably not possible, but this is besides the point).

Back in time I thought about workers usage in that library and I explored this part a bit (not that deep tho) and I don't see why it might be any issue tbh. The only real complexity that workers add (from my point of view) is that you need to ask a consumer to provide an URL to it to load it. Once it is loaded, its API is what you built and control from its entry point. Happy to be proven wrong tho.

I can push my initial changes to a branch if that will help, I mainly changed index.ts by removing the default export and exporting everything as named exports, and switched the getters and setters to be functions.

Yes please, it would help a lot. Also provide steps I need to do in order to validate that everything works (or at least to validate basic use cases) and if there are extra/untrivial steps to debug it at runtime (other than open browser/devtools and see/debug error 😂)

HarelM Jan 21, 2024
Author

The following is a branch I create with the changes:
https://github.com/maplibre/maplibre-gl-js/tree/imporve-exports
It's only a single commit which change the index.ts file.
If you want to test it locally you can npm i and then npm run start and surf to http://localhost:9966/test/examples/geojson-line.html if you see a line on a map it means that everything works correctly, which is not the current state because I removed the default export.
I think the default export is expected here:
https://github.com/maplibre/maplibre-gl-js/blob/0fc3baf23e6b7ce19f7d1de54b31bac1826769ed/build/rollup/bundle_prelude.js#L16
But I'm not 100% sure.

Yes, everything is done in rollup.

Thanks for looking into it!!

Also worth mentioning that there is a csp version of this library where the worker code and main code are separated in order to allow loading the main thread and the worker thread in parallel to improve load time, so there are customers that use this flavor.

I hope this answers your questions.

timocov Jan 21, 2024
Maintainer

It seems to work now: maplibre/maplibre-gl-js#3600

The solution is basically fix your custom define function as now rollup defines dependencies differently a bit and I just updated it.

Also worth mentioning that there is a csp version of this library where the worker code and main code are separated in order to allow loading the main thread and the worker thread in parallel to improve load time, so there are customers that use this flavor.

Frankly, initially I thought about this approach as the default one as it has more advantages and the cost of "few lines in the documentation" (get URL of worker module with consumer's bundler and pass it to setWorkerUrl function which seems quite trivial to do to me) 😂 I didn't expect to see a hack with URL.createObjectURL (which I didn't even think is possible lol, I like it).

Let me know if you have questions.

HarelM Jan 21, 2024
Author

Thank you very much for all the work!!! This is truly appreciated! I hope this will reduce the problems I have with both exported types and hopefully with the documentation (although the documentation doesn't seem to like the recent changes, I hope I'll be able to solve it).

timocov Jan 22, 2024
Maintainer

You're very welcome. Let me know if you'll have any issues or questions.

HarelM · 2024-01-24T06:47:30Z

HarelM
Jan 24, 2024
Author

@timocov one last question:
I saw that the tool is running validation on the generated d.ts file, is it possible to make this validation strict? is it already strict?
I'm currently doing a two phase build and check using a custom tsconfig just for the generated types. would be great if this could be incorporated into this tool.

6 replies

HarelM Jan 24, 2024
Author

I'm using the following tsconfig file on the generated d.ts file (this tsconfig is only there for this file):
https://github.com/maplibre/maplibre-gl-js/blob/main/tsconfig.dist.json
Unfortunately I can't make my typescript code adhere to the strict: true, but I do want my d.ts file to adhere to it.
Would be nice to be able to configure this in the tool configuration somehow.
Here's an example of an issue that I faced that needed this solution:
maplibre/maplibre-gl-js#934

Thanks!

timocov Jan 24, 2024
Maintainer

Are you sure that strict option has any effect on declaration files? As far as I know strict performs strict type check on your code, but declarations are just... types?

Anyway, you can compile your code for declaration files first (declaration: true) and then run the bundle tool on the output with a different config.

As a workaround you can put the output file to a folder where your "validation" config belongs, e.g. create a folder dts-out, move tsconfig.dist.json into that folder but rename it to just tsconfig.json, run dts-bundle-generator without providing project option (so it will try to find closest tsconfig.json file relative to a file it tries to compile) but with --out-file dts-out/index.d.ts option). It will force the tool to compile src/index.ts with /tsconfig.json, but dts-out/index.d.ts with dts-out/tsconfig.json. Folder name isn't exact, it is just to give an idea of the approach. I know it is not ideal, but at least it can unblock you right now.

I still think that compile for declaration files first and then bundle separately would be a better option for everyone (you might do similar stuff for js files if you would decide to perform some advanced optimizations on your code), but I think it is worth to consider this option so please cut a ticket in here.

HarelM Jan 24, 2024
Author

I'm sure strict is not only for code, as the issues linked here show.
I have a solution so I'm not blocked, which is a lot less complicated than what you wrote I believe:
I use tsc --project tsconfig.dist.json which only checks the d.ts file for errors as part of my CI (you can see it in the include list of this tsconfig).
But it would be great if this tool could do it out of the box so I can remove this tsconfig file and simply add --strcit or something to the execution of this tool.

I'll open a ticket then (I hope I got you right).

timocov Jan 24, 2024
Maintainer

But it would be great if this tool could do it out of the box so I can remove this tsconfig file and simply add --strcit or something to the execution of this tool.

Well you would still need a tsconfig file, I don't think it is worth to provide this very specific flag (next time other person comes and asks for another tsconfig flag and I will ended up with all tsc flags). It would be something like checkProject or checkTsconfig or something like that - a path to tsconfig that will be used for bundle validation.

I'll open a ticket then (I hope I got you right).

If the solution I just described works for you, please do.

HarelM Jan 24, 2024
Author

I thought I already read this answer, and apparently I did:
#187
Ho well...
I still think strict is very specific and has a reasoning behind it.
Your call.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make maplibre-gl-js more futureproof #291

{{title}}

Replies: 3 comments 11 replies

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Make maplibre-gl-js more futureproof #291

HarelM Jan 17, 2024

Option 1

Option 2

Option 3

Replies: 3 comments · 11 replies

timocov Jan 18, 2024 Maintainer

How it was done

The proposal

Some notes on --export-referenced-types flag

HarelM Jan 20, 2024 Author

timocov Jan 20, 2024 Maintainer

HarelM Jan 21, 2024 Author

timocov Jan 21, 2024 Maintainer

HarelM Jan 21, 2024 Author

timocov Jan 22, 2024 Maintainer

HarelM Jan 24, 2024 Author

HarelM Jan 24, 2024 Author

timocov Jan 24, 2024 Maintainer

HarelM Jan 24, 2024 Author

timocov Jan 24, 2024 Maintainer

HarelM Jan 24, 2024 Author

HarelM
Jan 17, 2024

Replies: 3 comments 11 replies

timocov
Jan 18, 2024
Maintainer

Some notes on `--export-referenced-types` flag

HarelM
Jan 20, 2024
Author

timocov Jan 20, 2024
Maintainer

HarelM Jan 21, 2024
Author

timocov Jan 21, 2024
Maintainer

HarelM Jan 21, 2024
Author

timocov Jan 22, 2024
Maintainer

HarelM
Jan 24, 2024
Author

HarelM Jan 24, 2024
Author

timocov Jan 24, 2024
Maintainer

HarelM Jan 24, 2024
Author

timocov Jan 24, 2024
Maintainer

HarelM Jan 24, 2024
Author