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

Use externs/olx.js instead of objectliterals.jsdoc #1954

Merged
merged 4 commits into from Apr 9, 2014
Merged

Use externs/olx.js instead of objectliterals.jsdoc #1954

merged 4 commits into from Apr 9, 2014

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Apr 4, 2014

The single externs/olx.js file describes all of the "options" objects we accept in our constructors. The @typedef annotations are used by the compiler for type checking. The @type annotations include documentation for individual options and serve as externs when compiling a profile of the library. When compiling an application together with the library, the externs/olx.js file is included as one of the sources to provide the @typedefs without generating externs.

If we want to maintain multiple src/*.externs.js files instead of one large externs/olx.js file, we can. But while we are still using Plovr, it makes for easier build configurations to have one file.

This removes the build tasks that generated the build/src/external/src/externs/types.js and build/src/internal/src/types.js files as those are both replaced by the single externs/olx.js file.

@tschaub
Copy link
Member Author

tschaub commented Apr 4, 2014

This is a work in progress. But comments are welcome (@elemoine).

There's code that can still be removed, and we'll need to do some JSDoc work (@ahocevar input welcome).

@tschaub
Copy link
Member Author

tschaub commented Apr 4, 2014

I should also mention that with these changes, the build/ol*.js builds are identical to builds in master. The build/examples/all.combined.js output is slightly different (323100 bytes here and 323046 in master). I haven't yet checked to see what the difference is.

@tschaub
Copy link
Member Author

tschaub commented Apr 4, 2014

The build/examples/all.combined.js file has 4 more unminified property names in this branch compared to master:

  • attribution - from olx.control.DefaultsOptions (twice)
  • keys - from olx.control.FullScreenOptions (twice)
  • minWidth - from olx.control.ScaleLineOptions (twice)
  • handle - from goog.fx.Dragger.prototype (three times)

I'm not sure why the compiler is not renaming those (it is successfully working with the use_types_for_optimization option elsewhere). It's also interesting to see the other properties that are not renamed (in either master or this branch). For example, zoom from olx.control.DefaultsOptions, controls from olx.MapOptions, and more.

@tschaub tschaub mentioned this pull request Apr 5, 2014
@tschaub
Copy link
Member Author

tschaub commented Apr 5, 2014

@ahocevar I started trying to get the @typedefs in externs/olx.js documented, but JSDoc is having trouble with our olx.control.MousePositionOptions (see jsdoc/jsdoc#619 - same issue using v3.2.2 or master).

Aside from the documentation issue, this is ready for review.

@tschaub tschaub mentioned this pull request Apr 8, 2014
@tschaub
Copy link
Member Author

tschaub commented Apr 8, 2014

@ahocevar - externs/olx.js generated from objectliterals.jsdoc @ aa6c188, so this is up to date as of now. I think any doc template or plugin changes should be based on @pagameba's work in #1964. If you push any relevant commits and reference them here, I can cherry pick them (assuming #1964 is merged).

@elemoine - let me know if you have any comments. My thinking is that we should go with a single externs/olx.js file as long as we are using Plovr. When we move away from that, it may be easier to manage multiple *.extern.js file in our src directory.

@ahocevar
Copy link
Member

ahocevar commented Apr 8, 2014

I'm working on getting types/typedefs documented with the new structure now, and will push to a branch that includes this branch and @pagameba's branch for #1964.

@ahocevar
Copy link
Member

ahocevar commented Apr 8, 2014

0b578df brings typedefs from olx.js back to the apidocs.

@tschaub
Copy link
Member Author

tschaub commented Apr 8, 2014

Thanks @ahocevar. I applied that commit here. I'd like to see #1964 and this merged. I've got a custom-build branch that also uses the Node version of JSDoc to extract @api annotations (or @todo api as long as Plovr is in the mix). I'd like to continue making progress on that based on these changes.

@elemoine any comments?

tschaub and others added 3 commits April 8, 2014 11:41
The single externs/olx.js file describes all of the "options" objects we accept in our constructors.  The @typedef annotations are used by the compiler for type checking.  The @type annotations include documentation for individual options and serve as externs when compiling a profile of the library.  When compiling an application together with the library, the externs/olx.js file is included as one of the sources to provide the @typedef's without generating externs.

If we want to maintain multiple src/*.externs.js files instead of one large externs/olx.js file, we can.  But while we are still using Plovr, it makes for easier build configurations to have one file.

This removes the build tasks that generated the build/src/external/src/externs/types.js and build/src/internal/src/types.js files as those are both replaced by the single externs/olx.js file.
@ahocevar
Copy link
Member

ahocevar commented Apr 8, 2014

Looks good to me!

@elemoine
Copy link
Member

elemoine commented Apr 8, 2014

I'll have a look in a few hours when I get back to my hotel room.

@@ -318,7 +298,7 @@ def action(t):
'inherits': '../../buildcfg/base.json',
'inputs': [
'../examples/%(id)s.js' % match.groupdict(),
'../build/src/internal/src/types.js',
'../externs/olx.js',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be hard to understand why an externs file is used as an input file here. I'd add a comment to explain this.

@elemoine
Copy link
Member

elemoine commented Apr 8, 2014

This looks good. Thanks a lot for the effort on this Tim!

I just added a few comments. One of them is related to using olx.js as an externs file for the compilation of the examples.

I'm also surprised that we do not need to use the keyword @interface when defining externs types in olx.js, but it's probably just me not remembering what we discussed some time ago. I'm happy to be said that we can defined externs using @typedef.

Please merge when you think this is ready.

@tschaub
Copy link
Member Author

tschaub commented Apr 9, 2014

using olx.js as an externs file for the compilation of the examples

Both examples-all.json and the generated build/examples/*.json configs specify their own externs and inputs (so don't acquire the same from base.json). So as I see it the examples aren't built with externs/olx.js as externs.

The externs/olx.js script includes @typedef annotations for objects that are used in the library.  When compiling the examples together with the library, we need these @typedef annotations.  In this same case, we don't need the object property names to be treated as externs (they can be safely renamed).
@tschaub
Copy link
Member Author

tschaub commented Apr 9, 2014

Thanks for the review @elemoine and @ahocevar. I added a comment to build.py about using externs/olx.js in the source when compiling the examples together with the library (3c92b69). I also added comments above about externs/olx.js not simultaneously being used as a source input and an externs. I hope this makes sense.

I'll send a writeup to the list about adding to olx.js when adding a constructor that takes an object literal. I think it would feel more natural to have these @typedefs closer to the source. I think we can make changes in this direction in the future.

tschaub added a commit that referenced this pull request Apr 9, 2014
Use externs/olx.js instead of objectliterals.jsdoc.
@tschaub tschaub merged commit 98ec656 into openlayers:master Apr 9, 2014
@tschaub tschaub deleted the olx branch April 9, 2014 02:36
@elemoine
Copy link
Member

elemoine commented Apr 9, 2014

Thanks a lot for the explanations @tschaub. It all makes sense.

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