Rename ol.feature.FeatureStyleFunction to ol.FeatureStyleFunction

[tschaub]

This puts ol.FeatureStyleFunction in the heap of ol function typedefs (along with ol.CanvasFunctionType, ol.CoordinateFormatType, ol.ImageLoadFunctionType, ol.PreRenderFunction, ol.TileLoadFunctionType, ol.TileUrlFunctionType, ol.TransformFunction).

This is really about avoiding problems with the API docs where a namespace matches a symbol in a case insensitive way (see #2149). But it also seems somewhat justified given the other *Function types.

Fixes #2149.

[elemoine]

+1

[ahocevar]

+1

[gberaudo]

What about removing completly the ol.FeatureStyleFunction and always use an ol.style.StyleFunction?

[tschaub]

What about removing completly the ol.FeatureStyleFunction and always use an ol.style.StyleFunction?

A ol.feature.FeatureStyleFunction takes one argument, and we've marked it as stable. A ol.style.StyleFunction takes two arguments and is marked as experimental. If we get rid of ol.feature.FeatureStyleFunction, we'd need to rework ol.style.StyleFunction so that this was a feature and it would accept one argument.

[gberaudo]

Since this PR breaks the stable API, it could have gone a bit further by completly getting rid of `ol.feature.FeatureStyleFunction`. I am wondering how an almost unused (according to your change note) method is marked API stable while the useful one is experimental.

[ahocevar]

It does not break the stable API. The signature of the function is unchanged, it just moved to a different namespace, which is irrelevant for a typedef.

[gberaudo]

The fully qualified name is changed, breaking all type casts to ol.feature.FeatureStyleFunction.

[elemoine]

I am wondering how an almost unused (according to your change note) method

The upgrade notes say "If you compile your application together with the library and use the ol.feature.FeatureStyleFunction type annotation (this should be extremely rare)".

What is rare is not the use of a feature style function, it is the use of the ol.feature.FeatureStyleFunction type, which assumes that your compile your application together with the library.

is marked API stable while the useful one is experimental.

They're both useful. The fact that one is marked stable while the other is not is a mistake.

[fredj]

The api stable ol.feature.FeatureStyleFunction typedef was renamed to ol.FeatureStyleFunction: technically speaking it breaks the stable api even if a type annotation should be rarely used.
Adding a note to changelog/upgrade-notes.md is perfectly fine by me (and everyone else I think).

Apart from the api discussion, there's absolutely no need for two typedefs: ol.style.StyleFunction could be used for both ol.layer.Vector and ol.Feature.

[elemoine]

Apart from the api discussion, there's absolutely no need for two typedefs: ol.style.StyleFunction could be used for both ol.layer.Vector and ol.Feature.

But the function signatures are different today, right? Are you suggesting that we should make them the same?

[fredj]

But the function signatures are different today, right? Are you suggesting that we should make them the same?

I'm suggesting (and it was also @gberaudo's suggestion) to remove ol.FeatureStyleFunction and use ol.style.StyleFunction instead

[tschaub]

We're all clear that there are two things. One is the name of the type and the other is the signature/behavior of the functions.

Regarding the name of the type

In my opinion, the API is the JavaScript API. When something is marked as stable, we promise that a minor release will not make you change the way you write JavaScript.

It is true that this change requires that people that compile their application together with the library need to change the comments in their code if they previously used @type {ol.feature.FeatureStyleFunction}. You could claim that our API extends beyond JavaScript to programming with comments, but I drew the line at a different place.

This perspective focusses on people who write applications using a built version of the library. I understand if others want to extend the definition of our API to the types used when compiling an application together with the library.

Regarding the function themselves

If we want to completely remove ol.FeatureStyleFunction in favor of ol.style.StyleFunction, we would have to change call sites to check the length of the user provided function before calling it to support both of the following:

layer.setStyle(function(feature, resolution) {
  // function length is 2
  // expects to be called with two args
});

feature.setStyle(function(resolution) {
  // function length is 1
  // expects to be called with `this` as the feature
});

I agree it would have been simpler to just have ol.style.StyleFunction. But breaking feature.setStyle(func) is a clear backwards incompatible change.

[gberaudo]

Thank you @tschaub for your detailed explanations.
It is fine if we break the API for good reason.

I implemented your suggestion in #3641.