Have actual functioning example in readme

cdrini commented 8 months ago

This example is so convoluted! We need to improve the setup flow. But this is the currently accurate setup :/

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 69.24%. Comparing base (2d19d55) to head (cb2db84). Report is 33 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1321 +/- ## ========================================== - Coverage 69.30% 69.24% -0.06% ========================================== Files 59 59 Lines 5082 5089 +7 Branches 1069 1072 +3 ========================================== + Hits 3522 3524 +2 - Misses 1533 1538 +5 Partials 27 27 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jrochkind commented 8 months ago

Huh. I have some questions

This example has a bunch of separate CDN-hosted scripts included, from unpkg.com -- this is how you actually do it in production?
- I would much rather use the packages from npm in a locally packaged bundle.
- That's what I was already doing for the BookReader-hosted stuff. Perhaps I will investigate trying to do it for the dependencies as well.
- But I want to confirm your actual production use of unpkg.com instead, and ask if there are any particular reasons to recommend this?
What is the polyfill stuff for? both polyfill.io/v3/polyfill.min.js and unpkg.com/lit@2.1.2/polyfill-support.js -- these do different things? And are both needed? To polyfill what?
We've got both @webcomponents/webcomponentsjs@2.2.10/webcomponents-bundle.js and @internetarchive/bookreader@5.0.0-80/BookReader/webcomponents-bundle.js -- these are not duplicates though? Do you know what the BookReader-bundled webcomponents-bundle.js is doing?

I guess if I'm going forward with path to include Book Reader only by iframe -- I'm bothered less by just blindly copying and pasting what you suggest here, since it will not effect the rest of my app. So if the answers to some of this are not at your fingertips, so be it! But I was curious to ask, I like to try to understand what I'm copy pasting and what it's doing.

cdrini commented 8 months ago

Thanks for the review @jrochkind !

You can use either; I thought the unpkg links might be easier for folks who might not want to create a local copy of the repo. The URLs will be the same, just replace https://unpkg.com/@internetarchive/bookreader@5.0.0-80 with the path to your local copy of BookReader. I'll add a note indicating as such! (On production we effectively have a local instance of unpkg running (although we use esm.sh there) and polyfill.io ).
Good question, I'll revisit to see if they're both still necessary that might be a residue from a past version. The polyfills are for supporting older browsers -- namely old iOS9 devices. This might not be a requirement for folks depending on what browsers they need to support. We tried to follow the guidelines here, but might be worth revisiting in the latest browser landscape: https://lit.dev/docs/v2/tools/requirements/#polyfills
Oh good catch! That's a typo, will patch up.

jrochkind commented 8 months ago

Thank you!

jrochkind commented 8 months ago

And, oh, re unpkg.com, it's not that I actually want to use local copies included with BookReader.

Rather, instead of doing (eg) <script src="https://unpkg.com/lit@2.1.2/polyfill-support.js">,

I would like to add to my local package.json, lit, "^ 2.1.2", and then import lit in my own JS files that I then bundle with vite (although it could be esbuild or webpacker, whatever). I'd like to use (eg) lit from it's own NPM package, that I manage the way I manage my other JS dependencies, rather than with a separate <script> tag to unpkg.com.

This makes it easier for me to manage all my dependencies, scan them for known vulnerabilities in certain versions, upgrade them, etc.

i don't really want to use versions that are bundled with bookreader though, and I don't plan to have a local copy of bookreader, right!

On the other hand, if these are kind of dependencies of bookreader, but bookreader doesn't actually specify them in it's own package.json, so it's hard to keep track of what versions might be compatible... I guess I have that problem either way, even with the <script> tags.

cdrini commented 8 months ago

Ah yes, the current bookreader flow bundles its dependencies; the use case being making it easy for folks who might not have their own build step. We have a line item on our roadmap this year to generate es6 compatible modules of bookreader; that'll enable your usecase where you want to run things through your own bundler.

jrochkind commented 8 months ago

I guess I probably CAN list the dependencies through my own bundler on my own now, instead of using unpkg.com -- I just need to keep track of version compatibility myself, either way in fact. Or maybe this won't work? I may experiment with it!

With the earlier approach from the README before this PR -- I was succesfully setting it up as an ES6 module already!

You do of course already express some dependencies in your own package.json. Maybe just not the "optional" ones like polyfills etc?

https://github.com/internetarchive/bookreader/blob/2d19d5537e3d5ecfe5532030cf6d679abd11709e/package.json#L28-L40

I am still curious what you are actually doing in production at archive.org itself, although maybe it doesn't exactly transfer being the main authors/use case of the reader who are responsible for determining version compatibility yourself in the first place!

OK, I think I just to experiment at this point. Although there's something to be said for just doing it exactly the way you suggest and have tested -- especially if I'm including via iframe isolation anyway -- but that's why I appreciate you confirming what parts of the README and examples actually ARE currently suggested and tested, instead of being out of date! Thank you!

cdrini commented 8 months ago

Can you give an example of what your es6 imports looked like? I think depending on what your bundler is doing that should also work in current bookreader. If you import from the BookReader/ dependency you will be bundling the already bundled/minimized js files, which will work, but will result in extra bloat. If you import from the src/ directory, you will get the 'classic' es6 experience. We do use some syntax that requires building (namely es6 decorators), but vite might handle that out of the box!

But yeah, for experimenting, I think either will work! We need to tighten up this flow on our end, too.

If you're curious, on prod we do something like this:

  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/BookReader.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.search.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.tts.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.url.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.autoplay.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.resume.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.archive_analytics.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.chapters.js');
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79/BookReader/plugins/plugin.text_selection.js');

// load the web component
  await import('{{INTERNAL_ESM_SERVER}}/@internetarchive/bookreader@5.0.0-79');

// other init code similar to the above

cdrini commented 8 months ago

^ And this code runs through a bundler on our production side (there it's esbuild). So we have the same bloating issue, which we need to resolve.

jrochkind commented 8 months ago

@cdrini

Those dynamic imports with await async are interesting and I don't completely get it, but ok! You don't import the polyfills there, or the webcomponent-bundle (which I think is also a polyfill?) -- not sure if you just didn't include those in your excerpted example, but are importing them similarly? Or handle those differently?

in my original proof of concept where I wasn't using the custom element, but just setting up the BookReader object pointing to a div container, I simply

import "@internetarchive/bookreader/BookReader/BookReader.js";

So I think that is along the same lines of what you are importing?

And then:

var br = new BookReader(options);

// Let's go!
br.init();

You can see that version of proof of concept code here: https://github.com/sciencehistory/scihist_digicoll/blob/b1d109a2aaf5f29eea42fc3fc7d29211be78b2ef/app/frontend/entrypoints/ia_book_reader.js

That also demonstrates my current custom getPageURI -- I think we were miscommunicating before, the thing is that my current derivatives aren't exactly proportional to 1/N for whole number N's -- they are instead things like "fixed 1200px no matter what the original size". So I'm not sure if a custom reduceSet like in #1317 is useful to me? Instead I just go with pow2, and calculate the closest derivative I've got that works. This seems to work out okay -- and if i return the same URL for different reduce values, the reader seems smart enough to use a cached version not refetch it!

jrochkind commented 8 months ago

OK, separetely... with the <ia-bookreader> component approach, I am still having trouble understanding how you associate the var br = new BookReader(options) instance with any or all of the <ia-bookreader> components.

The earlier demo-iiif.html example kind of made it look like storing it in window.br was like a "magic" location that would be used by all <ia-bookreader>, but I suspected I didn't have that right. The demo-internetarchive.html example, I just kind of have no idea.

This new example... it's also not clear to me... I haven't yet tried to exec it to see if it's actually working, I guess I probably should.

The previous README example I just had a div container, and mentioned that div container id in the el option to BookReader, I figured that's how they connected to each other?

In this new way of doing things, how is this new BookReader instance associated with one or all <ia-bookreader> custom elements, how do they find each other?

jrochkind commented 8 months ago

OK, I'm trying to get a version using the custom element working integrated into my app -- it is unfortunately giving me a lot more trouble than the approach outlined in this README (and most of the included examples) previous to this PR!

Original question answered about `window.br`

It looks like storing a BookReader object in the global variable at window.br is in fact the "api" as it were for the <ia-bookreader> custom element, it looks for such an object there and needs it to work?

A bit kiudgey and un-documented -- and it means you can't have multiple elements on a page that use different configuration? But who wants to do that anyway, and okay, it works! Would be nice to doc somewhere maybe.

Example in this PR, as is

if i save the example from the README in a file, and load it either locally as file:// or from a web server (seems to make no difference(

It works in Firefox or Chrome
Although my console does have an error for a 404 on trying to load file:///BookReader/images/loading.gif (or http://localhost:3000/BookReader/images/loading.gif), which of course is not there. Not sure if there's a good recommended way to deal with images -- or if there are OTHER image references that might wind up missing too?
In MACOS SAFARI I'm having more trouble.
- I'd be really curious if anyone else can reproduce, can you try this example in Safari?
- For me, I just have a "loading viewer" message with progress spiral that stays there forever. (If this is the loading.gif that was otherwise missing, that'd be... interesting)
- But oddly if I open the safari developer tools window... then it works?? What the heck? I swear this seems to reproduce for me. Makes it hard to debug of course.
- Still is 404'ing on the loading.gif

I haven't yet tested in any mobile browsers.

Some questions about the example

The styles in your example leave 100px of empty space in the window BELOW the Book Reader. Is this on purpose for some reason?

Trying to move on to a more locally bundled example

OK, the way this is done in the README with all these various separate <script> tags pointing to CDN's, and an inline <script> tag with the var br = new BookReader(options); etc setup code.... Is not how I would prefer to integrate this into and maintain this in my app.

I would really rather use a local bundler, where I have my own JS that does import (from npm package I have installed locally at deploy time, not a CDN at runtime) and setup in a file referenced by script src rather than inline.

So I'm trying to do that...

First thing for simplicity, I remove all the polyfill stuff. I do not believe it is necessary in any recent browsers, but just take out for simplicity for now.

I happen to be using vite, which helpfully will let me import a css file, and spit it out as properly loaded styles. Not sure if other bundlers can do similar. But mostly this seems pretty standard.

So that leaves me with a file something like this:

https://gist.github.com/jrochkind/1462c2166d94d436c7ad2da9e50d8017

And that does seem to work in Firefox and Chrome! (Similar problem in Safari as the original one-file README example!)

BUT it outputs a mysterious error in the console. The error doesn't seem to prevent the reader from rendering and behaving properly but it still worries me, like who knows if it is or would causing some problems I have not yet noticed/encountered.

It is hard to debug the error, because the versions of the JS src I am using from the npm package are already "minimized" so kind of inscrutable! But I wonder if this will ring any bells... Uncaught TypeError: Cannot read properties of undefined (reading 'identifier')

@internetarchive_bookreader_BookReader_ia-bookreader-bundle__js.js?v=9a269131:5357 Uncaught TypeError: Cannot read properties of undefined (reading 'identifier')
    at new e2 (@internetarchive_bookreader_BookReader_ia-bookreader-bundle__js.js?v=9a269131:5357:57)
    at l2.value (@internetarchive_bookreader_BookReader_ia-bookreader-bundle__js.js?v=9a269131:5861:36)
    at l2.value (@internetarchive_bookreader_BookReader_ia-bookreader-bundle__js.js?v=9a269131:5969:120)
    at @internetarchive_bookreader_BookReader_ia-bookreader-bundle__js.js?v=9a269131:5998:18
    at Set.forEach (<anonymous>)
    at @internetarchive_bookreader_BookReader_ia-bookreader-bundle__js.js?v=9a269131:5997:30

The top line of stack trace in inscrutable minimized JS is:

var a2 = null == n2 ? void 0 : n2.metadata, s2 = a2.identifier, l2 = a2.creator, c2 = a2.title, u2 = Array.isArray(l2) ? l2[0] : l2, d2 = i2.options.subPrefix || "";

Very curious if you have any idea at all what's going on here, or any ideas for how I could set things up differently to better debug it.

cdrini commented 8 months ago

Ahhh I see, there was a magic default! I'll update the example. It should be:

new BookReader({
    el: '#BookReader',
    ....
})

el defaults to #BookReader, so that's how it found the element!

Hmm I didn't test this code on safari! Let me give it a test there.

Oh and I fixed that metadata error you found in the latest release, so change to 5.0.0-80 and that should fix it!

Yep your setup looks perfectly reasonable, I'll update the example to include two versions; one with HTML and one with esm like yours does :+1:

cdrini commented 8 months ago

Oh and about the CSS yeah I need to untangle what's happening there :/ There are way too many variables! :P

internetarchive / bookreader