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

Two ways to use the library: advanced compilation (prod) or raw/concatenated (dev) #2404

Merged
merged 4 commits into from Jul 21, 2014

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Jul 18, 2014

This reduces the number of ways we support using the library. While it is possible to build the library with a dizzying array of options, it is designed to be used with ADVANCED_OPTIMIZATIONS in production. We also offer a way to debug the library during development.

These changes remove the following build targets:

  • build/ol-whitespace.js (replaced by build/ol-debug.js)
  • build/ol-simple.js
  • build/ol-all.js

The build/ol-debug.js target can be used as a single script for debugging (this was not the case with build/ol-whitespace.js). The build/ol-whitespace.js target generated 404s while trying to fetch deps.js. The build/ol-simple.js and build/ol-all.js targets were ways that we demonstrated we could exercise the Compiler, and provided no real value in my opinion.

We've proven that we can, but that doesn't mean we should be building with `SIMPLE_OPTIMIZATIONS`.
Instead of running everything through the Compiler just to remove whitespace, we provide a build that is a simple concatenation of all scripts in dependency order.  This build (ol-debug.js) should never be used in production (the same applies to the old ol-whitespace.js build).  Instead, the intention is that it be used to aid in debugging during development.
When viewing the hosted examples, people can load the compiled version of the library or load each script individually.
The ol-all.json and ol.json build configurations differ only in their use of the manage_closure_dependencies Compiler option.  I don't think there is value in running both (there are plenty of other compiler options that we don't exercise in this same way).
@ahocevar
Copy link
Member

Awesome! Please merge.

@probins
Copy link
Contributor

probins commented Jul 18, 2014

I'd agree that all these can be removed as part of CI (I've never been able to understand why Travis has to build them all every time), but I think there should continue to be a hosted simple build. Particularly when working remotely, and particularly on poor-quality connections, a raw build is far too big and unwieldy. To my mind, a simple build is a good compromise: much smaller than raw but easier to read/follow than advanced

@tschaub
Copy link
Member Author

tschaub commented Jul 18, 2014

@probins with apologies, I'm not compelled. There are two modes of operation I'd like to support: development and production.

For production, we have enough of a challenge ahead of us to provide hosted builds of the library (the full build even with advanced compilation is inappropriate for production). I don't want to mislead anybody into thinking that "simple" or "whitespace" compiled versions of the library should ever be used for production. Having one compiled target (that is still inappropriate for production) is enough.

For development, with this change we still have three ways to debug. First, we host an ol-debug.js script. This is very clearly named, and nobody should be mislead into thinking they can use it during production. Second, if you download a distribution and install dependencies, you can run a debug server. Third, if you are just poking around, our hosted examples can be run in "raw" mode.

@probins please continue to compile simple or whitespace builds for yourself if you find these useful. But I'm pretty firm on not encouraging anybody else to do the same.

@probins
Copy link
Contributor

probins commented Jul 19, 2014

ok, @tschaub, I understand your concerns. There is another possibility for helping developers debug their code, which is for the hosted builds to make source maps available for the advanced build. I use simple not primarily for debugging but to get non-obfuscated variable names. So another suggestion would be for the hosted builds to also have the renaming reports available online. None of these should be in Travis/ci target, just in the process creating the hosted advanced build.

@ahocevar
Copy link
Member

I think @probins's concerns can be handled separately, and I still think this should be merged. Just my 2¢.

@probins
Copy link
Contributor

probins commented Jul 21, 2014

I agree. I don't have a problem with this being merged. But I think there needs to be further thought on what builds apps developers will want to use and how.

@tschaub
Copy link
Member Author

tschaub commented Jul 21, 2014

Anybody who has tried debugging a compiled ol3 app with a source map will know that it is a horrible experience.

tschaub added a commit that referenced this pull request Jul 21, 2014
Two ways to use the library: advanced compilation (prod) or raw/concatenated (dev).
@tschaub tschaub merged commit 74dbd2b into openlayers:master Jul 21, 2014
@tschaub tschaub deleted the builds branch July 21, 2014 15:39
@probins probins mentioned this pull request Jul 22, 2014
@elemoine
Copy link
Member

For posterity: the goal of the buuld-all target was to throw all the source files to the compiler, to check that the entire code base compiles without errors.

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

4 participants