Dead code removal - Expression.serialize

[drwestco]

Each type of Expression has an implementation for a per-instance serialize method. From a cursory inspection of the project, these methods are only ever called from expression.test.ts - never from production code. Serialization of expressions via style properties is only done by serializing the raw values. Serialization across the web worker boundary behaves similarly - if a class implements a static serialize method, it will be called; if not, raw value serialization is performed.

There's a decent amount of code cleanup that can occur here, if all of these per-expression serialize methods can indeed be removed. Need to perform more thorough investigation to verify the code is dead, revise expression.test.ts, and then remove the serialize methods.

[drwestco]

History of Expression.serialize in the MB GL JS code: mapbox/mapbox-gl-js#6253

It's truly there only to support the tests, for parity with the same tests in the native codebase. I don't fully understand how a JS serialization implementation helps the native/JS workflow, and I certainly don't see the logic in releasing code that's never executed.

[wipfli]

Can you share an example of code that is not used?

[xabbu42]

There is code in maplibre-gl-native (https://github.com/maplibre/maplibre-gl-native/blob/ca5810f61fde26d0fbccbe4d48d4c771d02e460f/expression-test/expression_test_parser.cpp#L341) which uses the maplibre-gl-js tests for expressions. I did not have the time to dig in further or actually run that, but it seems we at least once had a common test suite for expressions in maplibre-gl-js and maplibre-gl-native. That would be highly valuable to ensure the compatibility of the two implementations and should not be given up lightly.

As I understand it so far, in maplibre-gl-native the serialize methods are used and need to be tested, but as it shares its test-suite with maplibre-gl-js there needs to be an implementation in both projects so the same tests run for both projects, or something like that.

[drwestco]

From what I understand, if the test.json doesn't have a section for the serialized representation, that step will be ignored. I think that's as it should be - there's no need for native's serialized format to match that of JS, especially since JS never actually serializes an expression!

There's additional testing that the output matches, both against the original parsed expression, and the expression after a round trip through serialization. That's also good, as it verifies expression compatibility between JS and native.

[HarelM]

Also, if this serialization code is needed in the native side (which I don't think it's the case) then it should be there.
The fact that both use the same library for test data is great for matching, but this was not changed in the linked PR.
In any case, I think this issue is resolved, feel free to continue the discussion.

[drwestco]

It looks like this portion of the native tests may now be broken, as it pulls the expected serialized format from the **/test.json files in the gl-js tree. https://github.com/maplibre/maplibre-gl-native/blob/ca5810f61fde26d0fbccbe4d48d4c771d02e460f/expression-test/expression_test_runner.cpp#L176

[HarelM]

But the expression is defined in the layer, isn't it?isn't it the same?

[HarelM]

Also note that native is using an older version of this repo, when this will be updated for the sharers (which is a huge effort) we'll probably need to take care of this as well.
But in theory, I don't mind having this property in the expected json file.

[drwestco]

Ah. I removed the "serialized" sections from each of the JSON files, since JS has no way to generate those now. Native test code should be updated to handle this. If it's important to verify the serialized output doesn't change, instead of just checking that the output is correct after round-tripping, that portion of the test data should be in the native repo.