Problem in generated ol-externs.js file

[elemoine]

With d9976ca#diff-497c3a47c9cdc970f191c9e4e852833c the generated ol-externs.js file includes multiple definitions of ol.format.WFS. There's a definition before the @typedef's for FeatureCollectionMetadata and TransactionResponse, and another one for the ol.format.WFS constructor itself.

@ahocevar, do you know where the problem comes from?

[gberaudo]

There are also three types whichl appear as unknown:
ol.format.Feature
ol.render.IVectorContext
ol.Sphere

[probins]

this seems to be related to the issues discussed in #2479

For the missing ones, see #2509 (comment) and #2359

[ahocevar]

For the duplicate of ol.format.WFS, I am fixing the generate-externs task. The other issues, as pointed out by @probins, are caused by inconsistent @api annotations.

[elemoine]

I'll see what I can do for the missing ones.

[ahocevar]

Reopening for the remaining issues.

[elemoine]

Some comments on the missing types:

• Add @api stable annotations to ol.format #2629 adds the @api annotation to ol.format.Feature.

• Documenting ol.Sphere #2359 includes comments about ol.Sphere. I think it's reasonable to add @api (experimental) to ol.Sphere for now. See Add @api to ol.Sphere #2641 for a PR.

• ol.render.IVector is an interface. Adding an @api annotation does not help, as the generate-externs.js task just adds a non-typed definition in that case:

  /**
   */
  ol.render.IVectorContext = function() {};
  

  So I'm not sure what to do for this one. @ahocevar, don't you think the generate-externs.js task should handle interfaces?

[ahocevar]

Thanks for the investigation @elemoine. I'll add support for interfaces to the generate-externs task. No need to add an @api annotation for interfaces.

[elemoine]

Thanks. With that, and #2629 and #2359, we should be able to use Closure Compiler's checkTypes flag.

[gberaudo]

There is also an issue with nullable returns; I created a distinct issue #2642 as it also impacts documentation.

[gberaudo]

The inheritance relation is also absent from the generated externs.

For example, ol.geom.Point extends ol.geom.Geometry, but the @extends tag is not present in the generated externs.

Currently, when I pass an ol.geom.Point, to a function taking an ol.geom.Geometry there is a check error.

[ahocevar]

I'll add the @extends annotation to the externs in a separate pull request.

[gberaudo]

Hi @ahocevar , there is a strange issue where ol.source.Vector extends ol.Observable instead of ol.source.Source in the generated externs. The documentation is correct, though. Could you please have a look to it?
All other inherited types are correct.

[ahocevar]

@gberaudo This is expected, because ol.source.Source is not exportable. The way the inheritance works in the generated externs is that if a parent class is not API, the parent's parent is used, and so on. If none is exported, the top of the inheritance chain is used, which is ol.Observable in this case.

[gberaudo]

@ahocevar, it seems strange to me to have an API type extend a non API type.

Here is the warning output by the compiler:
ERR! compile ol3/build/ol-externs.js:3008: WARNING - mismatch of the getSource property type and the type of the property it overrides from superclass ol.layer.Layer
ERR! compile original: function (this:ol.layer.Layer): (null|ol.source.Source)
ERR! compile override: function (this:ol.layer.Vector): (null|ol.source.Vector)
ERR! compile ol.layer.Vector.prototype.getSource = function() {};

It would be better if the generator could issue an error so that we can fix
these hierarchies. How many of them do we have?

Is there a reason not to mark ol.source.Source as API?

[ahocevar]

I think instead of marking ol.source.Source as API, I will go back to the previous idea and mark it @abstract. Pull request to come.

[elemoine]

Wait, it is weird that ol.source.Source is in the API docs while ol.source.Source is not an exported symbol.

For example, reading the API docs, I'd expect the following to evaluate to true:

layer.getSource() instanceof ol.source.Source

Instead, that throws a TypeError: TypeError: invalid 'instanceof' operand ol.source.Source.

[ahocevar]

What you are describing @elemoine is the result of ol.source.Source not being exported/exportable. And if you look at the API docs, you won't see the constructor. Only the exportable methods.

[ahocevar]

But I do see the problem, and a solution would be to make ol.source.Source exportable. So maybe we do want to make base classes exportable - also to allow users to subclass them? Not sure.

[elemoine]

But I do see the problem, and a solution would be to make ol.source.Source exportable. So maybe we do want to make base classes exportable - also to allow users to subclass them? Not sure.

Subclassing is another story. But yes the docs say that ol.source.Vector inherits from ol.source.Source, so I'd expect myVectorSource instanceof ol.source.Source to evaluate to true instead of throwing a TypeError.

[gberaudo]

Do we agree that ol.source.Source and more generally base classes should be exported so that we can do instanceof checks and they be present in the externs so that the compiler throws no error?

IMO, it would make sense to have the base classes in the API and documentation and to allow subclassing, but it is less important and we can decide on this particular point later.

[elemoine]

Do we agree that ol.source.Source and more generally base classes should be exported so that we can do instanceof checks and they be present in the externs so that the compiler throws no error?

Makes sense to me.

IMO, it would make sense to have the base classes in the API and documentation and to allow subclassing, but it is less important and we can decide on this particular point later.

Being able to subclass requires more than "exporting". It also requires externs to prevent renaming of overridable/overloadable methods.

[ahocevar]

Makes sense for instanceof checks, agreed.

[ahocevar]

Only one remaining issue here as far as I can tell. So if someone creates a pull request that exports the constructors of base classes, then I think we can close this.