search

readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
553 stars 186 forks source link

clean-css CLI and breaking changes in latest version #601

Closed hotTea closed 7 years ago

hotTea commented 7 years ago

This issue is a Bug

Expected Behaviour

'npm run dist' should complete successfully and generate distribution packages

Observed behaviour

'npm run dist' fails with the below error message

`> readium-js-viewer@0.25.0-alpha dist:cloudReaderLite:copy:fonts /mnt/home/readium-js-viewer

node ./readium-js/readium-shared-js/readium-cfi-js/node_modules/ncp/bin/ncp src/fonts/ dist/cloud-reader-lite/font-faces/

readium-js-viewer@0.25.0-alpha dist:cloudReaderLite:css /mnt/home/readium-js-viewer node ./readium-js/readium-shared-js/readium-cfi-js/node_modules/mkdirp/bin/cmd.js dist/cloud-reader-lite/css && npm run dist:cssoptimize -- "-o" "dist/cloud-reader-lite/css/readium-all.css"

readium-js-viewer@0.25.0-alpha dist:cssoptimize /mnt/home/readium-js-viewer node node_modules/clean-css/bin/cleancss --skip-rebase --skip-advanced --skip-aggressive-merging --skip-media-merging --skip-restructuring --skip-shorthand-compacting "node_modules/bootstrap/dist/css/bootstrap.css" "node_modules/bootstrap-accessibility-plugin/plugins/css/bootstrap-accessibility.css" "src/css/sourcesanspro.css" "src/css/readium_js.css" "src/css/viewer.css" "src/css/viewer_audio.css" "src/css/settings.css" "-o" "dist/cloud-reader-lite/css/readium-all.css"

module.js:327 throw err; ^

Error: Cannot find module '/mnt/home/readium-js-viewer/node_modules/clean-css/bin/cleancss' at Function.Module._resolveFilename (module.js:325:15) at Function.Module._load (module.js:276:25) at Function.Module.runMain (module.js:441:10) at startup (node.js:139:18) at node.js:968:3

`

Steps to reproduce

  1. Start fresh copy ie. directory where readium has not been already downloaded/installed.
  2. Download readium git clone --recursive -b develop https://github.com/readium/readium-js-viewer.git readium-js-viewer
  3. cd readium-js-viewer
  4. git submodule update --init --recursive
  5. git checkout develop && git submodule foreach --recursive "git checkout develop"
  6. npm run prepare
  7. npm run dist

Product

Additional information

It looks like the "bin" directory was removed from "clean-css" in the latest version (https://github.com/jakubpawlowicz/clean-css/tree/v4.0.2/), while it was present in earlier version (https://github.com/jakubpawlowicz/clean-css/tree/v3.4.24/bin)

If I update the version of "clean-css" package in https://github.com/readium/readium-js-viewer/blob/master/package/package_base.cson from "latest" to "3.4.24", everything works correctly again. So I am guessing either the "'dist:cssoptimize'" (https://github.com/readium/readium-js-viewer/blob/master/package/package_scripts_dist_base.cson) will need to be updated to reflect the new changes or the package dependency explicitly specified to the version where the "bin" directory still exist.

Please let me know if I am completely missing something. Thanks !

danielweck commented 7 years ago

Indeed, the bin entry in package.json has completely disappeared! https://github.com/jakubpawlowicz/clean-css/blob/v3.4.24/package.json#L19

danielweck commented 7 years ago

I will fix this regression bug by adding the new CleanCSS CLI: https://github.com/jakubpawlowicz/clean-css-cli

danielweck commented 7 years ago

Fixed here: https://github.com/readium/readium-js-viewer/commit/ea3d1199b30899304a8a2a0ad33e8930d47ed95a

Thanks for reporting this!

danielweck commented 7 years ago

I also addressed the breaking changes in the clean-css CLI API: https://github.com/readium/readium-js-viewer/commit/85cb909b02c29f3a261c3b1eff962fec52d87652

See: https://www.npmjs.com/package/clean-css-cli#important-40-breaking-changes

hotTea commented 7 years ago

Thanks @danielweck for taking care of this so fast ! I also wanted to ask, if you don't mind, is there a specific reason for configuring the third party libs to "latest" version, which could have breaking changes like the one in this ticket. Would it be any better if they were linked to specific versions where they are guaranteed to work for a given code version and maybe update the library version at some regular intervals ? Thanks again.

danielweck commented 7 years ago

Release tags on the master branch should definitely be tied to external dependencies with fixed / frozen semantic versioning tags, in my opinion (perhaps using NPM shrinkwrap). This way, a "stable" released build would never break. See issue: https://github.com/readium/readium-js-viewer/issues/432

The develop branch is constantly in flux, and a common practice is indeed to link to NPM modules with major changes only. However, ReadiumJS's development experience is that even supposedly non-breaking version increments did in fact break the build on some occasions. Furthermore, some of the NPM dependencies are GitHub repositories, pointing to branch tips / heads. So, apart from a few notable exceptions like Bootstrap, ReadiumJS references the NPM "latest" tag. Regression bugs due to API changes are usually caught rapidly, when the TravisCI servers (continuous integration) build the cloud reader and Chrome app. I wish Readium had a better regression testsuite though.

hotTea commented 7 years ago

That makes sense. Thanks for the explanation !

da70 commented 7 years ago

@hotTea Pegging dependencies to specific versions in the package.json file is unfortunately not enough to prevent build breaks due to dependencies upgrading. Even if Readium were to specify versions, there's no way to restrict the package.json files of the dependencies themselves -- they are free to update their own node_modules/. As Daniel said, it would take something like NPM shrinkwrap to fully lock a project, recursively. Unfortunately, npm seems to fail on a couple of the sub-modules.

We've been able to successfully lock our Readium instance using yarn. If you want to take a look, go to dlts-readium-js-viewer and check out the yarn.lock files for each Readium project (one in each sub-directory). The build script is in Bash and shows the commands you would run on the command line to build a locked-version reader. It's pretty simplistic, but it works. We do a (virtual) byte-level verification of the build, and so far there haven't been any problems.

@danielweck Our builder project also has selenium tests for our particular build. Not sure how interesting it would be for you, though, as the test suite is specific to our particular build which uses a plugin for customization, and also we are using the WebdriverIO library, but thought I'd mention it. EDIT: Tests are brittle, too...they were written for my office workstation and they pass there, but on home machine they don't.

danielweck commented 7 years ago

@da70 Thanks for sharing your Selenium / WebDriver + Chai acceptance tests ( https://github.com/NYULibraries/dlts-readium-js-viewer/tree/master/test/browser/tests ).

As you probably know, readium-js-viewer has tests too (Mocha), but the testsuite is really minimal: https://github.com/readium/readium-js-viewer/blob/develop/tests/tests.js

The tests can be executed locally, using WebDriver: https://github.com/readium/readium-js-viewer/blob/develop/package/package_scripts_test_base.cson#L6

SauceLabs is a bit flaky, but we have a script for that too: https://github.com/readium/readium-js-viewer/blob/develop/package/package_scripts_test_sauce.cson

Unfortunately when run through TravisCI (during automated Continuous Integration builds triggered at each Git push) SauceLabs regularly generated false negatives (interrupted builds) because of network / API timeouts, etc. So we disabled the tests for readium-js-viewer.

The readium-js-viewer TravisCI script still runs unit-tests for the CFI library though: https://github.com/readium/readium-js-viewer/blob/develop/.travis.yml#L78 https://github.com/readium/readium-js-viewer/blob/develop/package/package_scripts_travis.cson#L7 ...and the readium-cfi-js repository itself is of course configured to run the tests (Karma): https://github.com/readium/readium-cfi-js/blob/develop/.travis.yml#L21 https://github.com/readium/readium-cfi-js/tree/develop/tests https://github.com/readium/readium-cfi-js/blob/develop/package.cson#L308

da70 commented 7 years ago

@danielweck Thank you, yes when I first started thinking about our test suite I read through all the readium-js-viewer Selenium test code to get strategy and implementation ideas, and to understand the viewer interface better. Even though they are still in process, they were quite helpful.

We are not yet using Sauce Labs, but that's something I've been interested in exploring so looking at your configuration will be useful. Likewise we don't have CI set up for our Readium projects yet, but would like to do so eventually. The test suite we have now is a first draft and would almost certainly also generate a lot of false negatives on a CI server (even on my home machine, there are lots of failed tests)!

The readium-cfi-js unit tests run perfectly on my workstation.