Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat unknown opt types as Object in ol externs #2765

Merged
merged 1 commit into from
Sep 30, 2014
Merged

Treat unknown opt types as Object in ol externs #2765

merged 1 commit into from
Sep 30, 2014

Conversation

elemoine
Copy link
Member

As discussed previously we want to export base classes. This is to be able to do, for example, source instanceof ol.style.Image in applications. Exporting base classes means marking them with the @api annotation.

The problem is that, currently, if we just add the @api annotation to the base class we get an unknown type for that class' options type in the ol externs file:

/**
 * @constructor
 * @param {ol.style.ImageOptions} options
 */
ol.style.Image = function(options) {};

where ol.style.ImageOptions is not declared in the ol externs file.

One option involves adding @api to the options @typedef as well. This indeed fixes the problem in the ol externs file. But this creates a broken link in the API doc. In any way, it does not make sense to add @api to the options @typedef, because abstract base classes are not meant to be instantiated by applications.

This PR fixes the problem by making the generate-externs.js script use @typedef {Object} in the generated externs file for options types that are referenced in "exported" constructors but not marked with an @api annotation.

I have to admit that the code added to generate-externs.js is not of the prettiest, but I haven't been able to come up with a prettier solution.

Thanks for any review.

@gberaudo
Copy link
Member

+1 for this simple implementation.

Not having the option typedefs in olx for these particular cases make sense to me.
Indeed, only application code compiled together with ol3 may have a use of these options.

The reason, is that minification prevents base methods to be overridden by non-compiled code.

@elemoine
Copy link
Member Author

Options from olx namespace should not be automatically fixed as it is an error to not have a definition for them.

True. Changing…

@ahocevar
Copy link
Member

Looks safe to me. Please merge.

elemoine pushed a commit that referenced this pull request Sep 30, 2014
Treat unknown opt types as Object in ol externs
@elemoine elemoine merged commit 5d02760 into openlayers:master Sep 30, 2014
@elemoine elemoine deleted the ol-externs branch September 30, 2014 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants