Fix image expression

[ryanhamley]

Launch Checklist

• briefly describe the changes in this PR

This PR addresses two separate issues with the image expression operator.

• The changes in src/style address Inconsistent behaviours of “coalesce” with “image” #9630. Introduce within expression #9352 introduced an optional parameter in front of availableImages in the arguments list which accidentally broke Layout and Transitioning expression evaluation. This PR addresses that by adding the optional argument for those expression evaluation pathways.

• Fixing ⬆️led us to discover a separate issue in which image expressions on layers embedded directly into a style (rather than added at runtime) were being evaluated before availableImages was populated in the source's workers. The @mapbox/map-design-team reported the same issue in https://github.com/mapbox/mapbox-core-style-components/issues/917 although it wasn't clear at first that these two issues revealed the same bug.

• include before/after visuals or gifs if this PR includes visual changes

Before https://github.com/mapbox/mapbox-core-style-components/issues/917:

After:

Before #9630:
Note: The "not_existing" text label is a fallback to demonstrate the absence of the icon

After:

• write tests for all new functionality
• post benchmark scores
• manually test the debug page
• tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix 'image' expression evaluation which was broken under certain conditions</changelog>

[ryanhamley]

It's not clear to me why this existing render test would not have caught these bugs. The setup to for the test seems to be exactly what caused these issues but that test never failed.

[mourner]

• introduced an optional parameter in front of availableImages in the arguments list which accidentally broke Layout and Transitioning expression evaluation

One more reason to eliminate things like (this: any) from the codebase as much as possible — Flow could have caught this.

It's not clear to me why this existing render test would not have caught these bugs. The setup to for the test seems to be exactly what caused these issues but that test never failed.

Not sure but one guess would be that Node web workers mock is too fast to process messages (postMessage uses setImmediate), while a real browser takes a few ticks for the communication. One more good reason to explore render tests in the browser.

[mourner]

Looks good to me apart from the Flow errors. It would be great to get to the bottom of why the existing test passes though, to avoid similar regressions in the future.

[ryanhamley]

One more reason to eliminate things like (this: any) from the codebase as much as possible — Flow could have caught this.

Perhaps I'm misunderstanding Flow's documentation but https://flow.org/en/docs/types/functions/#toc-optional-parameters says that optional parameters can be missing. Is there any built-in way in Flow to avoid this problem if you have multiple optional parameters in a function signature?

EDIT: Actually, facebook/flow#2289 suggests that Flow should have caught this. It appears to me that Flow is somehow losing the context for possiblyEvaluate.

[ryanhamley]

My best guess on why the existing render test didn't fail is that Node was able to load and parse the local sprite sheet fast enough to cover up this bug. In production, it takes longer to load a sprite sheet over the network and parse potentially hundreds of sprites. A browser test could help with this but I'm not sure how we could make use of Selenium to determine if the correct image has been loaded.