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

zip libraries in both readium-js-viewer and readium-js, update all to latest version? #289

Closed
danielweck opened this issue Feb 23, 2015 · 8 comments

Comments

@danielweck
Copy link
Member

readium-js lib folder listing (the zip utility is used by the cloud reader to read EPUBs directly from the packed archive):
https://github.com/readium/readium-js/tree/develop/lib

readium-js-viewer lib folder listing (the zip utility is used to decompress/unpack the EPUB archives into the Chrome file system):
https://github.com/readium/readium-js-viewer/tree/master/lib/thirdparty

Right now, the files zip.js, zip-ext.js, zip-fs.js, deflate.js, and inflate.js are out of date. Also, z-worker.js is missing.

I guess these files should be updated in both repositories, consistently.

Dowloadable library package:
http://gildas-lormeau.github.io/zip.js/

Source code tree:
https://github.com/gildas-lormeau/zip.js/tree/master/WebContent

@danielweck
Copy link
Member Author

@danielweck danielweck changed the title zip libraries in both readium-js-viewer and readium-js, update all to latest? zip libraries in both readium-js-viewer and readium-js, update all to latest version? Feb 23, 2015
@danielweck
Copy link
Member Author

Using the aforementioned Pull Requests, I successfully tested the cloud reader with zipped EPUBs (bearing in mind the known caveats of audio/video/subtitle playback fail, see readium/readium-js#93 and readium/readium-js#94 ).

@danielweck
Copy link
Member Author

Note that @ryanackley implemented a fix that required modifying the zip library (BlobWriter), so we must preserve Ryan's changes in readium-js-viewer (I'm not sure about readium-js yet, as the EPUB loading strategy is different). See: 31e55f5#commitcomment-9873847
See:
#280

@danielweck
Copy link
Member Author

I've just noticed that because z-worker.js is missing, the cloud reader cannot load unpacked ePUBs anymore (develop branch). This is reproducible with both with the "proper" grunt cloudReader build (Almond'ed RequireJS bundles), and the index.html simple dev runtime (no parameter grunt => live RequireJS). So, it looks like the zip lib from readium-js-viewer is used instead of the one from readium-js (the latter still has the old zip lib version which did not require z-worker.js). Arghh! I look forward to complete the feature/plugins work which includes improving the RequireJS build, so that such lib discrepancies will not occur anymore. Meanwhile, my feature/zip-lib-update branch (mentioned above) seems to be the answer. I just need to make sure to include @ryanackley 's patch to BlobWriter.

danielweck added a commit that referenced this issue Mar 19, 2015
unpacked EPUB support in cloud reader (zip.fs is unchanged, to preserve Ryan's patches)
@danielweck
Copy link
Member Author

readium-js-viewer fixed here: 68c1ac7

readium-js still needs fixing, but the file zip.js must preserve @ryanackley 's Blob patch, to be consistent. This is not urgent, as the cloud reader builds include JS libs from the lib/thirdparty folder in readium-js-viewer (not from readium-js's own lib folder, even though the cloud reader "unpacked EPUB" feature is actually implemented there). Actually, an even better course of action is to complete the RequireJS refactoring, so that we can get eliminate issues with duplicated libs / inconsistent versions!

@danielweck
Copy link
Member Author

@ryanackley I have just successfully tested the big german EPUB that was making the Readium Chrome extension crash when importing it, it still works fine (I preserved your patch in zip.js, but updated the other files, see 68c1ac7 ).
The EPUB TestEpubThatCrashReadium.epub that was privately shared with us was from Niclas Bergstorm (ReadSpeaker). Could you please test the develop branch on your end with the EPUB that initially prompted you to implement the zip fix? Many thanks! Dan

@danielweck
Copy link
Member Author

Note that this is addressed in the "plugins" feature branch(es) for both readium-js and readium-js-viewer (no more duplicate zip-js lib, and we use Ryan's patched version obtained via an NPM package hosted on GitHub)

@danielweck
Copy link
Member Author

Fixed via RequireJS refactoring:
https://github.com/readium/readium-js/blob/develop/package.cson#L105

#    "zipjs": 'gildas-lormeau/zip.js'
#    "zipjs": 'ryanackley/zip.js'
    "zipjs": "danielweck/zip.js"

(we fork the zipjs lib to patch it, and we track upstream changes when invoking npm run prepare, just in case the author updates the original lib)

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

1 participant