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

Bundle more Google Closure Library #496

Closed
mfikes opened this issue Apr 17, 2017 · 8 comments
Closed

Bundle more Google Closure Library #496

mfikes opened this issue Apr 17, 2017 · 8 comments

Comments

@mfikes
Copy link
Member

mfikes commented Apr 17, 2017

Try to include nearly all of the current library.

@pyrmont
Copy link
Contributor

pyrmont commented Mar 9, 2019

Do you think this is still a goal? It seems to go against the spirit of a REPL to bundle a lot of dependencies.

@mfikes
Copy link
Member Author

mfikes commented Mar 9, 2019

@pyrmont I agree that, if there is a self-host library out there that can simply be consumed as a dependency, it shouldn't be bundled in Planck. (There are a few libraries, like Fipp and Transit, that Planck itself uses, so they need to be bundled—see https://github.com/planck-repl/planck/blob/master/site/src/dependencies.md#bundled-deps)

But, the Google Closure Library is different in that it is really underneath the standard core library, and there really is no opportunity to add it to Planck after the binary has been released. (Or at least, no easy way.) So ideally all of the Closure Library would be included.

This is really driven by all of the goog.* requires in https://github.com/planck-repl/planck/blob/master/planck-cljs/src/planck/bundle.cljs which I think may still be incomplete, hence this ticket.

@pyrmont
Copy link
Contributor

pyrmont commented Mar 10, 2019

That makes sense.

I don't know how helpful this is but to the extent that the issue is merely one of requiring the namespaces used in the Closure Library I thought I'd collect them all together so we had some sort of reference. I couldn't find a list easily so I downloaded the latest release, ran:

grep --include=\*.js --exclude=\*_test.js -hr '.' -e "goog.provide(" | sed -n "s/^goog.provide('\(.*\)');/\1/p" | sort -u

from the closure/goog/ directory of the repo and output it all to a file.

Here's the result as a Gist.

@mfikes
Copy link
Member Author

mfikes commented Mar 10, 2019

Oh wow. That is helpful. I would suspect we want nearly all of that, perhaps excluding testing namespaces.

@pyrmont
Copy link
Contributor

pyrmont commented Mar 17, 2019

@mfikes So I have a branch of Planck that compiles with the majority of the namespaces referred to in that Gist enabled. However, to make that work I had to delete a bunch of the namespaces as I was getting errors in the compilation step complaining that the file didn't exist. I looked more closely at how Google Closure Library (as distinct from Google Closure Compiler) is being brought in to the build process and it looks like you've put together a pre-compiled version that's hosted on the Planck website (currently this file). Is that correct?

What version of the Closure Library is that? Is there a particular reason to do it that way. How is it compiled? If I wanted to update the references in the build script with more up to date versions, could/should I do that?

@mfikes
Copy link
Member Author

mfikes commented Mar 17, 2019

@pyrmont That file is the JavaScript version of the Google Closure Compiler, ultimately derived from https://github.com/google/closure-compiler-js. It should be independent of the Google Closure Library. (At least that's my understanding.)

IIRC it is used to optimize files during the build, but also at runtime if you supply the -O / --optimizations option.

@pyrmont
Copy link
Contributor

pyrmont commented Mar 17, 2019

@mfikes Whoops! I misunderstood how the Google Closure Library was being pulled in. I've done some more investigation and I better understand the problem now.

The short version is that we're using a very old version of the GCL. Planck's build script relies on the version of the GCL that's brought in as a dependency with the ClojureScript artifact. That artifact is a non-offical distribution built by the ClojureScript team (presumably because there is no official distribution available through Maven).

I'm not sure why but the version depended upon by ClojureScript is version 20170809. There is a newer distribution (20190213) but it's only being used by Shadow CLJS.

In any event, the version Planck is building against is old. That means that the list of namespaces I generated based on the current version of the library includes namespaces that didn't exist in 2017; hence, the broken build process.

As I see it, there are two options:

  1. Use the 2017 version to generate the namespaces: This has the advantage of being simple to implement. It has the disadvantage that Planck will not provide access to an up to date version of the GCL and this may confuse users who can't find something that's supposedly 'in' the GCL (the documentation is not versioned so a user has no simple way to find out what is available in the 2017 version).

  2. Build against a newer version of the GCL: The advantage of doing this is that it avoids the problem described above. The disadvantage is that it's potentially much more complicated. I'm not even sure if this is something Planck can do or whether it requires an upstream change to the ClojureScript artifact. (The ClojureScript bootstrap script includes an option to build against the head of the GCL GitHub repo—maybe something similar can be done here.)

Do you have any thoughts on how to proceed?

@mfikes
Copy link
Member Author

mfikes commented Mar 17, 2019

@pyrmont We could easily update Planck to use the latest GCL by just specifying it as a dep. As an aside, Planck already has a fundamental needed change to support the latest GCL via 1c71f16.

You can prove to yourself that you have the latest GCL by evaluating goog/debugLoader_:

cljs.user=> goog/debugLoader_
#object[Object [object Object]]

(With the older GCL, this evaluates to nil.)

It is only a matter of time before ClojureScript itself updates to ship with a newer GCL; I see no huge problem with Planck doing this before ClojureScript proper does.

Here is an example diff needed to update to that version:

index a136b94..18f0ac3 100644
--- a/planck-cljs/deps.edn
+++ b/planck-cljs/deps.edn
@@ -1,4 +1,5 @@
 {:deps {org.clojure/clojurescript {:mvn/version "1.10.520"}
+        org.clojure/google-closure-library {:mvn/version "0.0-20190213-2033d5d9"}
         org.clojure/test.check {:mvn/version "0.10.0-alpha3"}
         com.cognitect/transit-cljs {:mvn/version "0.8.248"}
         fipp {:mvn/version "0.6.14"}
diff --git a/planck-cljs/src/planck/bundle.cljs b/planck-cljs/src/planck/bundle.cljs
index b1e7615..7cc1261 100644
--- a/planck-cljs/src/planck/bundle.cljs
+++ b/planck-cljs/src/planck/bundle.cljs
@@ -138,8 +138,6 @@
    [goog.string.newlines]
    [goog.string.newlines.Line]
    [goog.structs]
-   [goog.structs.AvlTree]
-   [goog.structs.AvlTree.Node]
    [goog.structs.CircularBuffer]
    [goog.structs.Heap]
    [goog.structs.InversionMap]

If you'd like to use the latest GCL with your updated bundle namespace changes, that would be cool. Feel free to PR with that.

With a PR like this, we'd probably want to update http://planck-repl.org/gcl.html in some way. (Maybe it makes sense to remove it, if we are practically shipping everything, or maybe it only makes sense to list what's not included instead of what is included.)

pyrmont added a commit to pyrmont/planck that referenced this issue Mar 19, 2019
Due to the way Planck is built, the Google Closure Library needs to be
bundled together with the executable. While the entirety of the library
has historically been available during the build process, only a small
subset has actually been included in the bundle.

This commit broadens the scope to almost the entirety of the Google
Closure Library (save for the facilities made available under the
`goog.demos` namespace). While this provides broader support to users,
it does substantially increase the amount of time it takes to complete
the full build process.

To provide an up to date version of the Google Closure Library, this
commit switches the version that is downloaded from the Maven repository
to a more recent distribution. This newer version of the Google Closure
Library caused conformance warnings to be displayed during the build
process. To avoid this, a newer version of the Google Closure Compiler
is also now required. As the newer version of the Google Closure Library
relies on features from ECMAScript 6, the language output of the Closure
Compiler has been switched to this version of ECMAScript.

Finally, the documentation has been updated as required.
pyrmont added a commit to pyrmont/planck that referenced this issue Mar 20, 2019
The initial commit included almost the entirety of the Google Closure
Library. Doing so increased the time taken to compile Planck
considerably. This commit removes some of the namespaces that are not
considered appropriate or desirable for Planck. This reduces the time
taken for a full build by about a third.

The documentation has also been updated.
pyrmont added a commit to pyrmont/planck that referenced this issue Mar 22, 2019
To improve build times for the optimised version of Planck,the previous
commit did not include the localisation namespaces that are part of the
Google Closure Library. This commit restores the namespaces and instead
changes the `planck-cljs/script/bundle` script to skip these files when
optimising.

This commit can now be considered to fix planck-repl#496. The documentation and
change log have been updated accordingly.
@mfikes mfikes closed this as completed in 88ee1fc Mar 22, 2019
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

2 participants