Use const enum more often to improve readability and reduce generated script size as byproduct

[zhangyiatmicrosoft]

Launch Checklist

I noticed that MapLibre in general prefers plain strings for message/status/state. This can result in repetitive strings everywhere and eventually the generated JS is more bloated than necessary.
Readibility is certainly a concern, but we can solve both problems with const enum, which is descriptive in TS and at compile time replaced with abbreviations in favor of script size.

In fact, plain strings are less friendly for readability and code searching -- for example the common word 'loaded' is being used by both tile state and RTLPluginState. If one were to search 'loaded', they both show up. This can be addressed by const enum as well

This draft PR is a staring point --- I only defined the const enum as an illustration. Will finish when everyone is ok with this direction.

Rough calculation: current MessageType has 21 keys, average length is 13. These keys are being repeated 46 times in the code base, which means 598 bytes.
With const enum of 2 or 3 chars, we can reduce script size by 500 bytes with just MessageType.

If this practice is widely adopted throughout the code base, noticeable size reduction can be archived. Thanks.

• 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.
• Document any changes to public APIs.
• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

I found enum in type typescript and javascript a total miss in terms of ease of use and ergonomics in general.
While it might prove to reduce bundle size I'm not sure it will be meaningful.
I would also like to avoid breaking the buplic API for this if possible.
The package is gzipped and this it might prove even smaller.
The globe for example adds like 20k IIRC, so I'm not 100% it's work the time, but, your call...

[zhangyiatmicrosoft]

I found enum in type typescript and javascript a total miss in terms of ease of use and ergonomics in general. While it might prove to reduce bundle size I'm not sure it will be meaningful. I would also like to avoid breaking the buplic API for this if possible. The package is gzipped and this it might prove even smaller. The globe for example adds like 20k IIRC, so I'm not 100% it's work the time, but, your call...

First of all we don't need change public API. Just the internal ones and when exposed, translate back to previous strings as needed.

Second, yes totally agree enum is very bad and hard to use.
"const enum" is different and they are great.
I like the most is that const enum is a pure TS thing, and completely replaced with plain values in compilation while maintaining readability. For example, for TS code:

/** Parameters for a vertex */
const enum VertexParameter
{
    /** Each vertex position data has 4 numbers: xyzw, */
    PositionElements = 4,

compiled JS looks like

for (var j = 0; j < 4 /* VertexParameter.PositionElements */; j++) {

so IMO it is the best for both worlds.

Agree after gzip, the changes look very trivial and negligible ... I am thinking everything adds up though, never know when it will suddenly reach a tipping point...
Also maybe just my pet peeves --- when it comes code searching, whether on GitHub pages or in VS code, the current plain texts generate too much noise. This problem is even more annoying when need debug a previous version of JS directly, where source map may be gone or too difficult to rewind source code to the exact commit.

[HarelM]

I don't object in general as long as we are not changing public APIs.

[zhangyiatmicrosoft]

By applying this change, the final script size is reduced by 644 bytes.
In order to take advantage of const enum, isolatedModules must be false. It is currently true and was added by this PR
https://github.com/maplibre/maplibre-gl-js/pull/1986/files

"isolatedModules": true,

This flag is intended for "...other transpilers only operate on a single file at a time". see more:
https://www.typescriptlang.org/tsconfig#isolatedModules

@HarelM @rotu are we expecting to ever use other compilers?

[HarelM]

I think we tried to yse swc to improve build time at some point in time, but it was too buggy.
In any case, if it's not needed right now, there's no reason to keep it, if needed, we can add it back in the future.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 88.80000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 86.74%. Comparing base (27d9c86) to head (08935c3).
Report is 2 commits behind head on main.

Files	Patch %	Lines
src/source/worker.ts	68.42%	0 Missing and 6 partials ⚠️
src/source/geojson_source.ts	66.66%	3 Missing ⚠️
src/util/ajax.ts	33.33%	2 Missing ⚠️
src/index.ts	50.00%	0 Missing and 1 partial ⚠️
src/style/style.ts	94.11%	1 Missing ⚠️
src/util/dispatcher.ts	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3879    +/-   ##
========================================
  Coverage   86.74%   86.74%            
========================================
  Files         241      242     +1     
  Lines       32355    32479   +124     
  Branches     1978     1992    +14     
========================================
+ Hits        28065    28174   +109     
- Misses       3360     3368     +8     
- Partials      930      937     +7     

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

[HarelM]

Can you check how this change affects the generated d.ts file?
How would isolatedModules affect consuming libraries?
This looks OK, although I'm not sure it's worth the effort given the changes that are expected to land in the coming version with the globe and projection...

[zhangyiatmicrosoft]

Can you check how this change affects the generated d.ts file? How would isolatedModules affect consuming libraries? This looks OK, although I'm not sure it's worth the effort given the changes that are expected to land in the coming version with the globe and projection...

• d.ts --- I am very surprised that things like MessageType are even in the generated d.ts, since people outside should not care and should not use it. In fact, it is impossible to do so.
  Nevertheless, I maintained the original name to minimize changes. Here is a sample PR to illustrate exactly what will be changed:
  https://github.com/zhangyiatmicrosoft/my-maplibre-gl-js/pull/9/files
  We should mark these internal APIs with @internal and TS will exclude them while generating d.ts, which is the best approach IMO. Let me know if you want to do that.

• How would isolatedModules affect consuming libraries? --- it does not. According to TS link shared earlier ...It does not change the behavior of your code, or otherwise change the behavior of TypeScript’s checking and emitting process.

• coming version with the globe and projection... --- is there a pending PR for that? Again, this PR is much more for the benefit of devs to improve readability, code searching, and debugging plain JS without sourcemap, which are very common scenarios. Script size reduction is only a byproduct. Future refactoring/renaming will be much easier as well. Think from this angle: improvements are made without regression.

[HarelM]

We should mark these internal APIs with @internal and TS will exclude them while generating d.ts,

I don't disagree, but the d.ts generation and the typedoc are not easy to please when it comes to internal and public stuff...

If the main driving force is dev experience then I'm all for it.

Globe PRs:

• Globe - fill layer #3882
• Globe - basic infrastructure, raster layer adaptation for globe #3783

[zhangyiatmicrosoft]

We should mark these internal APIs with @internal and TS will exclude them while generating d.ts,

I don't disagree, but the d.ts generation and the typedoc are not easy to please when it comes to internal and public stuff...

If the main driving force is dev experience then I'm all for it.

Globe PRs:

• Globe - fill layer #3882
• Globe - basic infrastructure, raster layer adaptation for globe #3783

Thanks. Separately I will probably investigate @internal.

[zhangyiatmicrosoft]

please merge at your conveniences. thanks