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

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

Open
danielweck opened this issue Nov 19, 2018 · 7 comments

Comments

@danielweck
Copy link
Member

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
Copy link

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
Copy link

aslagle commented Nov 19, 2018

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
Copy link
Member Author

@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
Copy link
Member Author

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):

public expressUse(pathf: string, func: express.Handler) {
this.expressApp.use(pathf, func);
}
public expressGet(paths: string[], func: express.Handler) {
this.expressApp.get(paths, func);
}

( https://github.com/readium/r2-streamer-js/blob/develop/src/http/server.ts#L136-L142 )

@danielweck
Copy link
Member Author

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:
#37

@danielweck
Copy link
Member Author

PS: I logged a separate issue about HTTP CORS preflight (options method):
#39
I've love your feedback / input :)

@aslagle
Copy link

aslagle commented Nov 19, 2018

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.

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

3 participants