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

Openlayers v3 #23

Closed
olivergeorge opened this issue Feb 4, 2015 · 25 comments
Closed

Openlayers v3 #23

olivergeorge opened this issue Feb 4, 2015 · 25 comments

Comments

@olivergeorge
Copy link
Contributor

Hello

I think this is vaguely what's needed for openlayers v3.

Key points

  • Seems like two downloads are required: the source zip has externs and the other has build/*js files
  • The externs might benefit from pruning, they include bootstrap, jquery, clojure-compiler...
  • The source zip can generate a suitable externs file using a node task

This doesn't build for me. Hoping you might get it going and include or perhaps just give me some pointers.

Update: A few tweaks (incorporated below) and now it builds. No guarantees beyond that just yet.

(set-env!
  :resource-paths #{"resources"}
  :dependencies '[[adzerk/bootlaces   "0.1.9" :scope "test"]
                  [cljsjs/boot-cljsjs "0.4.3" :scope "test"]])

(require '[adzerk.bootlaces :refer :all]
         '[cljsjs.boot-cljsjs.packaging :refer :all])

(def +version+ "3.1.1-0")
(bootlaces! +version+)

(task-options!
 pom  {:project     'cljsjs/ol3
       :version     +version+
       :description "openlayers.js v3 packaged up with Google Closure externs"
       :url         "http://d3js.org/"
       :scm         {:url "https://github.com/cljsjs/packages"}
       :license     {"BSD" "http://opensource.org/licenses/BSD-3-Clause"}})

(deftask package []
  (comp

    ; NOTE: Source repo has externs
    (download :url "https://github.com/openlayers/ol3/archive/v3.1.1.zip"
                     :unzip true)

    ; NOTE: Binary repo has build files
    (download :url "https://github.com/openlayers/ol3/releases/download/v3.1.1/v3.1.1.zip"
              :unzip true)
    (sift :move {
                 ; NOTE: this might not be a suitable format
                 ; #"^build/ol-debug.js" "cljsjs/development/ol3.inc.js"
                 #"^v3.1.1/build/ol.js" "cljsjs/production/ol3.min.inc.js"})

    (sift :include #{#"^cljsjs"})
    (deps-cljs :name "cljsjs.ol3")))
@olivergeorge
Copy link
Contributor Author

NOTE: It's possible that their ol-debug.js file might be better left out

@olivergeorge
Copy link
Contributor Author

It looks like you can generate a suitable externs file using a script in the ol3 source zip.

node tasks/generate-externs.js ol3.ext.js

@martinklepsch
Copy link
Member

Thanks for looking into this. :)

; NOTE: this might not be a suitable format
; #"^build/ol-debug.js" "cljsjs/development/ol3.inc.js"

Is the OpenLayers lib written in Google Closure? If so I've previously seen -debug variants that don't go well with Clojurescript already providing various goog.* namespaces.

It looks like you can generate a suitable externs file using a script in the ol3 source zip.

I think if we can get the externs directly from the publisher of the lib we should do that.

Feel free to open a PR with this any time.

@olivergeorge
Copy link
Contributor Author

Thanks, great to hear back from you.

Regarding ol-debug.js, yes, looks like their google closure code. I'll ignore.

I might need a few clues on how to build node tasks/generate-externs.js ol3.ext.js into the build.boot

Other than that I imagine it'll fall into place. My other stumbling block was clojurescript version related but I'll setup a fresh start for the PR.

@martinklepsch
Copy link
Member

@olivergeorge you can take a look at how firebase is packaged, it's also a Closure lib.

I might need a few clues on how to build node tasks/generate-externs.js ol3.ext.js into the build.boot

If I understood you correctly we can also download the extern file from one of the archives you mentioned, right? If that's the case I'd prefer to use the file that is provided by the library authors.

PS. referring to this:

    ; NOTE: Source repo has externs
    (download :url "https://github.com/openlayers/ol3/archive/v3.1.1.zip"
                     :unzip true)

@olivergeorge
Copy link
Contributor Author

Sorry, that was ambiguous wasn't it.

The source repo doesn't have an extern file to copy but it does have a node task which can generate one. (Wish I knew that a few days ago!)

Two, actually three, options as best as I can see it:

  1. Assume the externs doesn't change often and include it by hand (not a long term solution)
  2. Run the node task from boot, then grab the generated file.
  3. Start with New Jar structure #1 but ask the openlayers maintainer to include the extern file generation before packaging up the release so it's ready for us.

@olivergeorge
Copy link
Contributor Author

On that third point, I figured I'd log an issue in case it's a quick fix.

openlayers/openlayers#3202

@martinklepsch
Copy link
Member

I like 1+3 the most because it keeps the packaging simple and consistent with other packages.
@olivergeorge I logged an issue (see above).

@martinklepsch
Copy link
Member

Alright, closed mine again. haha.

@olivergeorge
Copy link
Contributor Author

hah, cool. I'll pull together a PR.

@martinklepsch
Copy link
Member

Maybe add a comment with a note about generating externs + a link to the OpenLayers ticket to the build.boot file
openlayers/openlayers#3202

@olivergeorge
Copy link
Contributor Author

Wow that went cleanly. Hope it looks okay...

#24

@olivergeorge
Copy link
Contributor Author

Awesome

I'll stick a few simple examples in a repo while I'm playing around. Few
sharp edges might be worth migrating into the README.

First example just uses the raw JS from their getting started...

https://github.com/condense/example_openlayers_cljsjs/blob/master/hellool/src/hellool/examples.js

On 5 February 2015 at 04:17, Martin Klepsch notifications@github.com
wrote:

Closed #23 #23.


Reply to this email directly or view it on GitHub
#23 (comment).

@jeluard
Copy link
Contributor

jeluard commented Feb 4, 2015

Great stuff!

@olivergeorge do you have a CLJS example?
Especially I am curious how well closure compiler eliminates dead code given how big OL3 is. Would there be a difference if it was integrated as a closure dependency?

@olivergeorge
Copy link
Contributor Author

I think I can work that out. brb...

On 5 February 2015 at 10:21, Julien Eluard notifications@github.com wrote:

Great stuff!

@olivergeorge https://github.com/olivergeorge do you have a CLJS
example?
Especially I am curious how well closure compiler eliminates dead code
given how big OL3 is. Would there be a difference if it was integrated as a
closure dependency?


Reply to this email directly or view it on GitHub
#23 (comment).

@martinklepsch
Copy link
Member

If we can add it as a closure library that would be fantastic!

On Thursday, February 5, 2015, Oliver George notifications@github.com
wrote:

I think I can work that out. brb...

On 5 February 2015 at 10:21, Julien Eluard <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Great stuff!

@olivergeorge https://github.com/olivergeorge do you have a CLJS
example?
Especially I am curious how well closure compiler eliminates dead code
given how big OL3 is. Would there be a difference if it was integrated
as a
closure dependency?


Reply to this email directly or view it on GitHub
#23 (comment).


Reply to this email directly or view it on GitHub
#23 (comment).


Martin
http://martinklepsch.org http://martinklepsch.org?ref=email

@jeluard
Copy link
Contributor

jeluard commented Feb 4, 2015

Does that change something besides having to rely on import and imposing a different syntax? Also does cljsjs help with closure library support?
I used to have a package for lein-cljsbuild but it has been broken by some changes in how ClojureScript discovers closure libraries.

@martinklepsch
Copy link
Member

I'm not sure if this is how it currently works but I guess having a :libs
key inside deps.cljs would be the way to go.

On Thursday, February 5, 2015, Julien Eluard notifications@github.com
wrote:

Does that change something besides having to rely on import and imposing
a different syntax? Also does cljsjs help with closure library support?
I used to have a package https://github.com/jeluard/cljs-ol3js for
lein-cljsbuild but it has been broken by some changes in how
ClojureScript discovers closure libraries.


Reply to this email directly or view it on GitHub
#23 (comment).


Martin
http://martinklepsch.org http://martinklepsch.org?ref=email

@olivergeorge
Copy link
Contributor Author

This is an experiment in compiling openlayers v3 into your app directly. There are file size benefits to this. Based on the getting started example provided by OL3 the filesize is improved from 539K to 247K when we switch out the cljsjs/openlayers library and compile openlayers ourselves.

https://github.com/condense/example_openlayers_cljsjs/tree/master/compiledin

Openlayers is built on Google Closure but doesn't follow the source file conventions required for compilation with cljs. Turns out it's two problems: file names don't match package provided, and files providing multiple namespaces.

Through sheer bloody mindedness I proved it can work. I scripted a "move file to location of goog.provides script" and manually teased apart the multi provide files. Hours later, it worked! Problem is that this new codebase is hard to keep up to date because of the file splitting.

End result, I pinged the OL3 dev list to share my experiences and ask about willingness to receive patches. Not sure it's a priority for them.

https://groups.google.com/forum/#!topic/ol3-dev/TyhQqM-GlBM

Please treat all of this with suspicion. Others with more experience of compiling using Google Closure and the clojurescript compiler can talk with much more authority.

@jeluard
Copy link
Contributor

jeluard commented Feb 5, 2015

Great job! 👍
So it looks like in advanced compilation dead code elimination is only performed for closure based libraries? Your original size is pretty much OL3 size.
Final size after your change is a great positive sign now I am pretty sure with the old cljsbuild way I could go much lower. Not sure why?

Another way to use OL3 in closure mode would be to modify the ClojureScript compiler so that it discover closure modules even when they don't follow folder convention. Not sure if that's feasible.

@olivergeorge
Copy link
Contributor Author

Yeah, it's clear that the compiler can operate in that manner else OL3
wouldn't build at all.

I presume it's just that the java style approach (file matches package) is
more secure and predictable design decision. Perhaps for cljsjs it is
useful to be able to compile with more relaxed assumptions but that's above
my paygrade. I look forward to seeing what you decide and others think.

On 5 February 2015 at 11:32, Julien Eluard notifications@github.com wrote:

Great job! [image: 👍]
So it looks like in advanced compilation dead code elimination is only
performed for closure based libraries? Your original size is pretty much
OL3 size.
Final size after your change is a great positive sign now I am pretty sure
with the old cljsbuild way I could go much lower. Not sure why?

Another way to use OL3 in closure mode would be to modify the
ClojureScript compiler so that it discover closure modules even when they
don't follow folder convention. Not sure if that's feasible.


Reply to this email directly or view it on GitHub
#23 (comment).

@jeluard
Copy link
Contributor

jeluard commented Feb 5, 2015

My opinion as a non-contributor is that there is more value to have OL3 as a regular JS dependency than nothing.
Now its size is pretty big and it would be great to have it closurified at some point.

Maybe we can keep adding relevant updates to this issue to keep a trace?

@olivergeorge
Copy link
Contributor Author

Sounds like a plan. I imagine Martin has thoughts on the matter.

On 5 February 2015 at 12:06, Julien Eluard notifications@github.com wrote:

My opinion as a non-contributor is that there is more value to have OL3 as
a regular JS dependency than nothing.
Now its size is pretty big and it would be great to have it closurified at
some point.

Maybe we can keep adding relevant updates to this issue to keep a trace?


Reply to this email directly or view it on GitHub
#23 (comment).

@olivergeorge
Copy link
Contributor Author

Got some feedback from @elemoine from OL3.

Sounds like they are happy to include the extern in releases which is great.

openlayers/openlayers#3202 (comment)

My long shot asking about coding conventions / refactoring to meet our stricter code layout requirements seems to be a dead end.

https://groups.google.com/forum/#!searchin/ol3-dev/oliver/ol3-dev/TyhQqM-GlBM/oPOxVFIb3egJ

@martinklepsch
Copy link
Member

Very much agree about everything that has been said here:

  • regular JS lib is better than nothing
  • if possible we should leverage Closure compiler

I've opened another issue to discuss this: #28.

I'll poke around how/if this can be done.

Let's continue the discussion in the new issue so this doesn't get lost in a closed one.

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

No branches or pull requests

3 participants