Replace node assert with normal error handling

[birkskyum]

Hint: chrome extension "Refined Github" allow you to hide/unhide resolved comments.

Instead of having the node assert, which are only in the debug build, this PR is either removing the statement if it's unnecessary, or replacing it with throwing a normal error. It would potentially move us quite far towards supporting ecmascript modules.

Instead of having a big 5kb node assert library bundled to the browser with browserify in debug mode, and transpiling everything out of the codebase for prodcution builds, I was thinking about what we needed. Turns out all our assert statements are very simple boolean checks, so we can readily replace them with if statements.

This would mean that it's not server specific code anymore, so browserify is out. unassert-rollup-plugin is uncessesary too. These checks now stay in the production code, adding only ~1kB, and they improve our error handling quite a lot.

This is obviously adding some lines of code to production, but the benchmarks shown here (left is 2.3, middle is main, right is this branch) says that we don't loose performance because of it.

[github-actions]

Bundle size report:

Size Change: +1.47 kB
Total Size Before: 203 kB
Total Size After: 204 kB

Output file	Before	After	Change
maplibre-gl.js	193 kB	195 kB	+1.47 kB
maplibre-gl.css	9.65 kB	9.65 kB	0 B

ℹ️ View Details

No major changes

[HarelM]

The only main "issue" I see work this approach is that there is a functionally change in this commit: before this code simply didn't exist in production so if there was an assert that might be triggered but didn't cause any actual error the user wouldn't know as opposed to now where there might be "throws" that cause actual problems.
Not sure how to address this though besides maybe a global variable that indicate of this is a debug build... Maybe...

[birkskyum]

All the debug pages seem to just work, so for the most part it is likely a trivial update. But because it technically is more strict, to comply with semver we could make it a 3.0.0-pre.1 and get some feedback.

Currently I distinguish if it's a debug build by looking at the filename, checking this expect(minBundle.includes('canary debug run')).toBeFalsy(); or peeking if the code has been through terser, but there could be an easier way similar to how this map.version is exposed. But is it more important for this release than the prior somehow - maybe I'm overlooking something.

[HarelM]

I would like to keep functionality as much as possible and not create a breaking change version just for this.
Is there's a way to declare a global variable somehow using rollup or something similar so that this will not run in production but only in development it would be nice...
We could also look at the usage themselves and see which are actually needed and which will give an error anyway and reduce the assertion usage... IDK...

[birkskyum]

@HarelM , phew, I think that was all of them and the naiveAssert is removed as well. To me it now points more in the direction of a 2.3.1-pre.1 , because it's not intended to be breaking, api-changing or adding any features.

[wipfli]

I am really impressed by the number of comments. You already are at like 50 percent of #209...

[HarelM]

:-) I'll make a final review later on tonight. This will need a pre-release to allow other to test it...

[HarelM]

I've added a few last comment.

[birkskyum]

I've added a few last comment.

Thanks! These adjustment are in place now.

[HarelM]

Great work!

[birkskyum]

For good measure, the benchmark didn't show any regression over the past 40 commits. Ignore the first column 2.3.0 (bug in benchmark).

[wipfli]

Cool, thanks for removing these dependencies @birkskyum

[birkskyum]

Thank you, but I was merely following @HarelM 's guidance throughout :) Hope this will give us some interesting side benefits.

[birkskyum]

Thanks, but it was merely following @HarelM 's guidance throughout (thank!) :) Hope this will give us some interesting side benefit like the option to use an unbundled dev workflow should we want to.

[wipfli]

Needed for maplibre/maplibre-gl-js-docs#230 I think.