readium / r2-streamer-js

NodeJS Readium2 "streamer"
BSD 3-Clause "New" or "Revised" License
21 stars 10 forks source link

Configurable features (not just API/constructor parameters, maybe config file?) #38

Open danielweck opened 6 years ago

danielweck commented 6 years ago

For example, to disable the built-in sample "readers" (NYPL demo, etc.), HTTP CORS (in some cases they are in fact unwanted), OPDS micro-services, etc. etc. (there are many optional features, perhaps the server constructor should launch by default with no HTTP route activated, and a convenience method on the server class could implement a baseline suitable for "readium desktop", another for the Heroku/Now.sh online deployment, etc.)

JayPanoz commented 6 years ago

re config file: this article might be a good illustration – that can at least help people checking this issue understand how it looks like.

aslagle commented 6 years ago

I would also like to be able to use some of the routes from another server, or add some of my own routes to the streamer server.

Here's what I did to make that work with an older version of the streamer: https://github.com/NYPL-Simplified/opds-web-client/blob/master/packages/server/index.js

danielweck commented 6 years ago

@JayPanoz of course I agree with using runtime environment variables, specifically in a pure-server-side situation that is totally sandboxed / controlled by the server operators. However, historically we have relied on a minimal number of ENV variables in the Readium2 "desktop" app (for which ; thus far ; the r2-streamer-js component has primarily been developed for) because they require careful elimination at production time when shipping to end-users (in order to close potential security holes). In essence, the build process (currently WebPack) rewrites the ENV vars as if they were "preprocessor directive" / macros (C/C++ lingo), by making them static immutable values, baked-in at compile time (same with the DEBUG flags).

danielweck commented 6 years ago

Thanks @aslagle, useful info! :)

Looking at the NYPL Git history ( https://github.com/NYPL-Simplified/opds-web-client/commits/master/packages/server/index.js ), I see that the "server" adaptations were put into place as early as Feb. 2016. To be honest, this is the first time I hear about this (sorry if I have missed conversations). Did you raise any issues about r2-streamer-js at the time?

FYI, the current r2-streamer-js "server" API includes use and get wrapper functions to hide the Express app internals (we could add options, post equivalents ... or simply add a getter method to expose Express to the outside world): https://github.com/readium/r2-streamer-js/blob/c1f3ac0023efc31ad03f0a41c59568eecff0dc61/src/http/server.ts#L136-L142 ( https://github.com/readium/r2-streamer-js/blob/develop/src/http/server.ts#L136-L142 )

danielweck commented 6 years ago

Side note: I had a chat with our JelloBooks friends earlier today (including @JayPanoz). They have integration requirements that go beyond improving the API / architecture here and there (such as adding/removing routes, controlling HTTP headers, inserting auth middleware, manipulating the server's internal state ; e.g. load/unload publications, etc.).

For example the r2-streamer-js internals would need to be carefully refactored in order to evolve from publications "unique identifiers" (i.e. the base64 URL path segment) expressed using filesystem paths and remote HTTP locations, to ; for example ; UUIDs (internally mapped to something else), or custom URL protocols (e.g. database URI schemes), etc. In this case the ramifications are broader and deeper in the r2-streamer-js codebase, perhaps even also in consuming clients (I'm not sure yet, it depends where/how the current base64-encoded path is used).

Related issue: https://github.com/readium/r2-streamer-js/issues/37

danielweck commented 6 years ago

PS: I logged a separate issue about HTTP CORS preflight (options method): https://github.com/readium/r2-streamer-js/issues/39 I've love your feedback / input :)

aslagle commented 6 years ago

I think I did mention on some of the calls that I thought the streamer had too many features that made it difficult to use, but since I was only using it for a demo I set up that workaround to only take the part I wanted. My opinion would be that optional features like the sample readers and OPDS routes shouldn't be in this server at all. I'd rather have a simple streamer with a way to add additional features, and maybe a "test app"-style server with the sample readers. The OPDS stuff could get its own package that could optionally be added.