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

[HarelM]

Bug report

Input code

See the difference of the Map object between version 4.0.0-pre.2 and 4.0.0-pre.3 of our library here:
https://unpkg.com/[email protected]/dist/maplibre-gl.d.ts
https://unpkg.com/[email protected]/dist/maplibre-gl.d.ts

Expected output

Map object should be exported as Map and not as Map$1

Actual output

Map is exported as Map$1.
This is basically an unintended break as the object that should be exported is Map and not Map$1

Additional context
I don't think there're a lot of changes to this library beside the upgrade version version 8.1.2 to 9.2.1, so I tend to think this is the root cause of this.
It seems like this should have been solved by a bug fix in version 9.2 according to the changelog, but I don't think it solved this specific case.

Let me know if you need more info in order to reproduce this issue.
The repo can be found here:
https://github.com/maplibre/maplibre-gl-js

Thanks for all the hard work put into this library! Truly appreciated!

[timocov]

Might be related to #286. The handling of collisions with "global" types was added in 9.2 so hopefully downgrading to 9.1 should help as a workaround.

[timocov]

It is not a bug in the tool, this line in the build scripts is the issue https://github.com/maplibre/maplibre-gl-js/blob/b364c529d63d39012e28e7e1a4882e1c413d2a7d/build/generate-typings.ts#L14 (basically adding export keyword to any class regardless of its name 😂):

types = types.replace(/declare class/g, 'export declare class');

The tool now can rename items in the generated bundle to avoid collisions, including collisions with global types (from lib or other types that contribute to global) e.g. Map or Event. Is there any particular reason why you don't export these classes explicitly?

[HarelM]

Well, I kinda do export them...
This hack that I added (only after the tool runs BTW) is to avoid problems I encountered in the past.
I'll try to explain:
the Map class is one of the properties of the global object of this library (this global object exists for historical reasons, unfortunately):
https://github.com/maplibre/maplibre-gl-js/blob/b364c529d63d39012e28e7e1a4882e1c413d2a7d/src/index.ts#L46
This static global object is exported as default:
https://github.com/maplibre/maplibre-gl-js/blob/b364c529d63d39012e28e7e1a4882e1c413d2a7d/src/index.ts#L271

So one should be able to:

import maplibregl from 'maplibre-gl';
const map = new maplibregl.Map(...);

but also:

import {Map} from 'maplibre-gl';
const map = new Map(...);

Both are valid and should work, I just don't know how to make the tool understand that.
It's worth mentioning that I have the same issues with typedoc for the same reason, the library exports types and classes in terms of typescript, but these are not part of the "public API".

I'm struggling to find a good solution for both problems.
See here:
TypeStrong/typedoc#2469

Any help would be appreciated.

[Atrue]

@HarelM, I wrote my thought here #286 (comment)

But you can try to think of it from another perspective. Let’s imagine you didn’t have this bundle. So it means your index.ts file https://github.com/maplibre/maplibre-gl-js/blob/b364c529d63d39012e28e7e1a4882e1c413d2a7d/src/index.ts#L271 exports the only default variable MapLibreGL.

If you look at your index.test.ts file, you import it is the same way - as a default https://github.com/maplibre/maplibre-gl-js/blob/b364c529d63d39012e28e7e1a4882e1c413d2a7d/src/index.test.ts#L2
So you can’t write something like this in your tests:

import {Map} from ‘./index’;
const map = new Map(...);

How to achieve this without the bundle? Just export everything before exporting the default variable in the index.ts file:

export { Map, NavigationControl, ... };
export defaul MapLibreGLt;

So the idea is if you write working code without the bundle, the bundle makes the compatible code and everything will work without any additional manipulations.

[HarelM]

:-) rollup complains that you are exporting in two different ways.
I tied that here:
maplibre/maplibre-gl-js@8cfa6fc
I found a solution but I don't know if I like it.
See here:
https://github.com/maplibre/maplibre-gl-js/pull/3564/files
I can simply

export {
    type Map
}

But then I need to export things that are basically already exported and this creates duplication in maintenance...
See my comment in the linked issue:

• 4.0.0-pre.3 does not export Map in typescript maplibre/maplibre-gl-js#3563

[Atrue]

@HarelM Exporting types is not the same as exporting classes.
TS will throw an error when you try to use it like this:

import { Map } from 'maplibre-gl';
const map = new Map(); // 'Map' cannot be used as a value because it was exported using 'export type'.

You have to export it as it if you want to reach the same behaviour. Or you have to drop named import support

[HarelM]

The type itself exists in the d.ts file, it does not get exported.
Adding the export type helps to overcome this issue.
It's more of a hack to overcome what I tend to think is a limitation of the tool than an actual solution.
I might be wrong though...

[Atrue]

@HarelM I see, I showed you an error TS throws when using wrong defined d.ts file. So it does not affect the JS result, it’s only the TS error. But it’s not what you’ll expect.

What I’m trying to say is your code should be valid before applying any bundle. Currently, it exports the only default class, but you are expecting it exporting named variables as well. To reach this you need to write a valid index.ts file first (you can check it with index.test.ts file for example) and check the bundlers (maybe it requires some changes in rollup) to compile it to the expected module format.

[HarelM]

I agree with what you are saying, I just don't know how to solve this properly, the export type is just a hack I guess...
The problem is that the index.ts file is not the last part of the build, as the build also bundles some worker code and ships those together to allow some code to run in the worker thread without the need of a developer intervention.
This complexity along with the history of the library (originally written in js + flow and had a manually defined definition file in definitely typed) is what causing me a headache...
I honestly am not sure what should be the right solution here...
If I export both default and named exports, rollup isn't happy, and I'm not sure it's wrong. It might be that the solution is to remove the default export and keep only the named exports, but that's a pain to all the users using this library and also makes things harder to consume this library "as is" (without bundlers) which is what we offer in our example page, see here:
https://maplibre.org/maplibre-gl-js/docs/examples/simple-map/
Bottom line, a hot mess...

[Atrue]

@HarelM I’d suggest you to drop default export in the future releases and figure out the issue with rollup.
Right now as a quick fix you can create a new file types.ts and use it for dts-bundle-generator and write something like this:

import MapLibreGL from './index'; // import default class

// export all named classes
export {Map} from './ui/map';
export {NavigationControl} from './ui/control/navigation_control';

export default MapLibreGL; // re-export default class

[HarelM]

Currently, I've solved it by exporting the type of all the missing classes as can be seen here:
maplibre/maplibre-gl-js#3564
I'm not proud of what I did, but it solves the issue.
If I remove the default export I'll need all my users to change how they work with the library, which isn't a great DX.
also "global methods" will look "strange" instead of maplibregl.workerCount = 4 the users will need to import {setWorkerCount} from 'maplibre-gl' and other "weird" stuff...
I'm planning a major breaking change version right now and I'm in pre-4 of it, so I can break things, I'm just not sure how to break them in the "right direction"...

[timocov]

@HarelM Just wanted to note that after #290 export type will be transformed to export type (currently the tool changes it to just export). If in your workaround you're relying on this it might stop working as it produces an incorrect output.

[HarelM]

I'm struggling to get it right. I'm not sure what I should do honestly.
Everything I do ends up being a workaround that breaks in the next major version that comes out.
Please direct me what I should be doing to avoid needing to change my code every time a new version comes out.

[timocov]

breaks in the next major version that comes out.

Yeah I see your confusion, but to be honest export type thing is a bug (thanks for finding it btw! 😄) and probably will go as a minor release.

Please direct me what I should be doing to avoid needing to change my code every time a new version comes out.

@HarelM in case if you can do a breaking change, what you can't sacrifice 100%? Its hard to suggest anything if you need to keep somewhat "weird" old-days style of exporting API in JS. I can understand the reasoning behind why it was like that years ago, but now it feels a bit out.

I'm ready to help you with this journey from my experience and point of view, we can have some dialog either here or you can create another discussion/issue in your repo or so - as you wish. I agree with what @Atrue was saying above, and I sympathize the suggestion of getting rid of default export as it cases all sorts of problems (or can cause them) as very first step. Secondly, I'd remove class MapLibreGL and export fields/methods you declare there as separate exports. After that we can re-visit what you'd get and see if there is any point of improvement. But again, I don't know what use-cases you have and what you need to maintain no matter what.

[timocov]

If I remove the default export I'll need all my users to change how they work with the library, which isn't a great DX.

Re this - can you provide examples of what use-cases you'd absolutely need to keep? I'm not familiar with your library so to provide any good advice I need to understand its usage more.

which isn't a great DX.

I see your point and I 100% agree that a library author shouldn't generate unnecessary changes that break things up and make DX worse for no reason, but this is kinda slippery slope situation - instead of making a breaking change at "early stages" and re-design the API surface you can ended up keeping "old way of designing the API because users used to use it this way". It is not necessary bad tho, but it might make your life worse (or your users).

also "global methods" will look "strange" instead of maplibregl.workerCount = 4 the users will need to import {setWorkerCount} from 'maplibre-gl' and other "weird" stuff...

Again, I'm not familiar with the library and its internals, but from my experience while globals might be useful, at the end of the day they cause so much pain - e.g. if you need to maintain several different instances in your application your global values might be different between them and it might be helpful to scope them to an instance; also there is a chance of introducing memory leaks if you accidentally store some objects that shouldn't be there. It might not be your case of course, and these changes are huge.

If I remove the default export I'll need all my users to change how they work with the library, which isn't a great DX.

To this, it seems (based on what I read in this thread) that what you wrote in index.ts isn't necessary what your users will see in the result bundle so you might be doing something in your bundle that changes the output. It would be helpful if you can provide different types of how your library can be and is used. E.g. how your users are usually import it, create an instance of main class(es)/call functions, etc. This might be a compound problem that might affect bundling process to make it simpler and by removing some steps from bundling/post-processing into the compilation/export step.

[HarelM]

I opened the following discussion:

• Make maplibre-gl-js more futureproof #291