Require all when generating exports #6207

Merged
merged 1 commit into from Dec 6, 2016

Projects

None yet

4 participants

@tschaub
Member
tschaub commented Dec 4, 2016 edited

Our externs/olx.js depends on symbols that might not be explicitly required in a build. For example, the first typedef in olx.js assumes that the ol.TileRange type is known to the Compiler. If a build doesn't depend on any tile layers, the Compiler would complain about an unknown type when it encountered ol.TileRange in olx.js.

One solution for this is to pass all source files to the Compiler (instead of only those that are explicitly required by something in a custom build). The Compiler should still get rid of unused code in this case, but it will also know about all symbols before coming across them in olx.js.

This branch proposes that we include a goog.require() for everything that is goog.provide()d by a module with an @api annotation. Note that this is different than passing all files to the compiler. This would fail if we referenced a symbol in olx.js that wasn't a dependency of anything exportable. I can't think of any reason to have code in the library that is not a dependency of an exportable symbol, so I don't imagine this will be a problem. And the generate-exports.js script is a convenient place to write out goog.require()s since we already have a list of goog.provide()d names for every exportable symbol. Plus the generated exports file informs the Compiler to ignore extraRequire, so we don't need to tell everybody to add this to the jscomp_off section of their build config for things to work.

@tschaub tschaub Require all when generating exports
99eddbb
@gberaudo
Member
gberaudo commented Dec 5, 2016

Have you tried the first solution of passing every js to the compiler?

It looks the most appealing to me since it would not rely on magics in the build tools.

@tschaub
Member
tschaub commented Dec 5, 2016 edited

There are two places where we can make changes: our tasks/build.js and dependent scripts or closure-util. A third alternative would be to not use closure-util at all.

Currently, we rely on closure-util to generate a list of files to pass to the compiler. Our build.js and related scripts produce an exports.js script that "seeds" the dependency tree - this file includes goog.require() calls for everything that is to be exported in a build. We get back a list of dependencies from closure-util, and we pass those along to the compiler.

Though the idea of "passing every js to the compiler" sounds straightforward, there is more involved - particularly if we want existing build configurations to keep working. For example, it is currently possible to depend on files in the Closure Library (and our library itself still depends on base.js). So, "passing every js" is not just about passing OpenLayers files to the compiler.

In this branch, I have made it so that the exports.js that we use to seed the dependency tree includes goog.require() calls for every file that has exportable symbols (instead of just those that are to be exported in a custom build). I'd consider this a fairly minor change that should make it so custom builds continue to work without any additional changes.

An alternative to this approach would be to explicitly add every file in our source and vendor directories to the list of dependencies we get back from closure-util. In addition to requiring a change to our build.js script (which might also be characterized as magic), I think this would require that everybody update their build configurations to suppress the extraRequire errors generated by the compiler.

If anybody has a concrete suggestion on a alternative approach, I'm open to it. I think we should work away from closure-util longer term, but I think we can have an interim fix that let's people continue using it for a while.

@tschaub tschaub added this to the v3.20.0 milestone Dec 6, 2016
@gberaudo
Member
gberaudo commented Dec 6, 2016

Thank you @tschaub for your detailed explanation and insight on your longer term goal, you have convinced me.

I share your view that we should go away from closure-util in the longer term. We have done some experiments and it looks doable. On a pilot project, we rely on the depswriter.py script for the unit tests and for debugging. I did some experiments with compilation (project js + ol3 js + goog.asserts/provide/require; it is almost working. We are also looking into using goog.module (or directly ES6 modules) for our project code.

That is why I was concerned about your move: if there is some subtle difference inside the complier between just passing the js and requiring the symbols, then it would be a rock on our road.

But, since we are sharing a common goal, I am confident we will be able to work out solutions.

+1 for merging your PR.

@sbrunner this PR will probably impact the ngeo build tool. Do you want to have a look?

@ahocevar

My custom builds continue to work after this change, and the approach taken here makes sense to me as well. Thanks, and please merge.

@tschaub tschaub merged commit 82c0dcd into openlayers:master Dec 6, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 86.596%
Details
@tschaub tschaub deleted the tschaub:require-all branch Dec 6, 2016
@probins
Contributor
probins commented Dec 8, 2016

there is one side-effect of this: the manifest file the compiler can output is no longer useful, as it now lists all source files instead of just the ones required by the app. Not a major issue; I just have to find another way to list dependencies. I can probably use closurebuilder.py if I fiddle around a bit.

@adube adube referenced this pull request in camptocamp/ngeo Dec 13, 2016
Open

WIP - Upgrade to OpenLayers v3.20.0 #2109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment