Update to official closure compiler plugin

[funkyboy]

This: https://www.npmjs.com/package/google-closure-compiler

Otherwise the build on CI fails, because the current one looks for the jar file in the wrong path

[funkyboy]

This is blocking for the parallelization in #452

[SimonWoolf]

why does the ci build need the closure compiler at all? The only thing we use the closure compiler for is minification, and iirc the tests use the non-minified lib so that we get understandable stacktraces if the lib crashes

[paddybyers]

I assume it's because all grunt tasks (including running tests) have a dependency on the build task, which in turn needs the closure compiler.

[funkyboy]

If you don’t run nodeunit before karma tests you end up here
https://travis-ci.org/ably/ably-js/jobs/338615662

[funkyboy]

@SimonWoolf @paddybyers speaking of which. Any arguments against committing the .jar directly to the repo? That would allow consistent builds both locally and on the CI.

[paddybyers]

Any arguments against committing the .jar directly to the repo?

It's now available via npm: https://www.npmjs.com/package/google-closure-compiler

Shouldn't we update to that (ie add it to devDependencies) and remove the curl, unzip tasks?

[funkyboy]

Yep, that’s what I linked above, my first proposal.

[paddybyers]

Yep, that’s what I linked above, my first proposal.

ah, ok ;) I think we should do this.

[funkyboy]

I just hope it’s a matter of swapping a package with another. said the dreamer

[paddybyers]

Try this: #463

[funkyboy]

@SimonWoolf @paddybyers I removed the compiler altogether. We have this: https://travis-ci.org/ably/ably-js/jobs/338925797#L721 but tests work with no problem :)

[paddybyers]

I removed the compiler altogether. We have this: https://travis-ci.org/ably/ably-js/jobs/338925797#L721 but tests work with no problem :)

Yes, but that's not really the point of my PR. We will continue to need the closure compiler for the minified versions of the library. My PR updates that. Then are you going to run tests on that version of the library (ie that package.json) or a different one?

[funkyboy]

@paddybyers Sorry, should have been more explicit :) I propose to remove the compiler. After all it's needed only to make the official release. WDYT?

[paddybyers]

I propose to remove the compiler. After all it's needed only to make the official release. WDYT?

I don't know whether or not it is used as-is, bundled as part of the static assets of the library. Creating the minified libs could just be part of the deploy pipeline. But then what if we want to run tests using those minified versions? What if a 3rd party wants to modify/fix the library and wants the resulting minified version? I don't really see what we gain by removing it but I'm open to discussion.

@SimonWoolf ?

[funkyboy]

@paddybyers @SimonWoolf I just discovered that we can't run too many BrowserStack in parallel.
So this is not a blocker anymore.
I'd leave this open. Let's talk about when/how to update dependencies in the tech catchup.

[paddybyers]

I just discovered that we can't run too many BrowserStack in parallel.
So this is not a blocker anymore.

I don't understand the relationship between the two issues

[funkyboy]

@paddybyers when I ran BS and nodeunit tests in parallel that meant they were all run on different machines. Running BS tests on a "machine" without running nodeunit first led to the "missing closure compiler" issue.

[paddybyers]

OK. Well the fact remains that we should either remove, or update, the support for the closure compiler in this library. I think it's valid to have a discussion about whether it really belongs as part of the deploy pipeline, but what we have today seems to be broken, and we should do something about that.

[funkyboy]

I agree that we should update it.
I'd first run tests on minified JS before updating the compiler. Feels safer. WDYT?

[paddybyers]

Feels safer. WDYT?

Yes, definitely. So far all I did is verify that it installs and generates a minified script.

[funkyboy]

Fixed in #463