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

Provide embedded http server #22

Open
wants to merge 3 commits into
base: feat/pagedjs
from

Conversation

Projects
None yet
3 participants
@oncletom
Copy link
Collaborator

oncletom commented Nov 30, 2018

live from https://www.pagedmedia.org/pagedmedia-prepostprint-workshop/

  • port is selected randomly
  • it removes the manual task of starting the server
  • it prevents the "fetch() cannot run local files" error
@Mogztter

This comment has been minimized.

Copy link
Owner

Mogztter commented Nov 30, 2018

Thanks @oncletom
For reference, here's the reply from the Paged Media about replacing fetch with XMLHttpRequest: https://gitlab.pagedmedia.org/tools/pagedjs/issues/86#note_480

Thats interesting that it works different then fetch, I'll try it out and make sure everything works ok with the switch.
However I'd recommend just running an express server for the files rather then allowing access to the whole filesystem like we do in pagedjs-cli: https://gitlab.pagedmedia.org/tools/pagedjs-cli/blob/master/index.js#L48

So in the meantime using an embedded server is the way to go.
Any objection @matklad @lordofthejars @mojavelinux ?

Having said that we could revisit this issue when/if Paged.js switch to XMLHttpRequest where we could choose to lower the security of the Chrome headless instance (instead of using an embedded server).

@oncletom

This comment has been minimized.

Copy link
Collaborator

oncletom commented Nov 30, 2018

Exactly, this proposal is a "workaround" of the fetch/XMLHttpRequest situation.

I opted for a lightweight set of modules. The server is closed once the PDF generation is done.

One possible next step as an improvement would be to stream the PDF generation to the server instead of capturing the whole output in memory.

@matklad

This comment has been minimized.

Copy link
Contributor

matklad commented Nov 30, 2018

This looks awesome to me! Perhaps there should be a flag to disable/enable this behavior? Would it be possible to make it work with the --watch option as expected?

@oncletom

This comment has been minimized.

Copy link
Collaborator

oncletom commented Nov 30, 2018

Ah sure!

Perhaps there should be a flag to disable/enable this behavior?

Yes. I'd eventually couple it with a --base-url so as localhost:5000 is not hardcoded, and can be changed to whatever is the user local setup.

Would it be possible to make it work with the --watch option as expected?

What do you mean? To start the server only with --watch?

Let me know for both cases, I'm happy to make the changes.

@matklad

This comment has been minimized.

Copy link
Contributor

matklad commented Nov 30, 2018

Oh, sorry, looks like I completely misunderstood what this PR is doing =/

I though this was not for generating PDF which refer to local resources, but for a more streamlined dev workflow. That is, I thought that this opens a browser windows with HTML for the purposes of displaying a rendered document.

So, my comment about --watch was for a completely orthogonal issue. Currently, to get live-reload I run /asciidoctorjs-pdf in one terminal and reload -b in another (where reload is some npm module for live reloading). I with that this workflow was built-in. Though, this is not a big deal, b/c I can create it locally.

@oncletom

This comment has been minimized.

Copy link
Collaborator

oncletom commented Nov 30, 2018

Understood.

This PR is solely to make the generation of the PDF possible without launching an HTTP server aside. This HTTP server is not meant to be user facing, but accessible to the headless browser to create a safe environment for the paged.js library.

At the moment, if you don't launch an HTTP server on localhost:5000, the PDF generation will fail.

@Mogztter

This comment has been minimized.

Copy link
Owner

Mogztter commented Nov 30, 2018

At the moment, if you don't launch an HTTP server on localhost:5000, the PDF generation will fail.

on the feat/pagejs branch 😉

I opted for a lightweight set of modules. The server is closed once the PDF generation is done.

👍

Would it be possible to make it work with the --watch option as expected?

I think we should start the HTTP server only once if we use --watch. If I'm not mistaken, currently the HTTP server will be opened and closed for each generation ? What do you think ?

@oncletom

This comment has been minimized.

Copy link
Collaborator

oncletom commented Dec 3, 2018

Yeah right, it's only on the Paged.js branch 😅

Server is pretty fast to start and close. I'm happy to do the change but not sure if that's necessary to do it now.

I added a new thing in afa1590, the ability to proxy 3 files (asciidoctor.css, document.css and pagedjs.polyfill.js to the package files). This way the assets are always loaded relatively to the original source document.

I did that because I had an issue while generating a PDF document from an Asciidoc files located outside of the project file structure.

const getPort = require('get-port')
const handler = require('serve-handler')
const {createServer} = require('http')
const debug = util.debuglog('asciidoctor-pdf')

This comment has been minimized.

@oncletom

oncletom Dec 3, 2018

Collaborator

This way, debug logs appear when doing NODE_DEBUG=asciidoctor-pdf ./bin/asciidoctorjs-pdf ...


const url = `http://localhost:${port}/${tempFilename}`;
const httpOptions = {
public: workingDir,

This comment has been minimized.

@oncletom

oncletom Dec 3, 2018

Collaborator

With this, public assets are relative to the original document. It prevents the need for relativeFilePath and baseAbsolutePath .

debug(`start server on localhost:${port}`)

const server = createServer(async (req, res) => {
if (req.url in STATIC_ASSETS) {

This comment has been minimized.

@oncletom

oncletom Dec 3, 2018

Collaborator

I was confused by serve-handler's rewrites options. It looks like this works only with files located within its public directory.

page.on('error', reject)

try {
debug(`goto ${url}`)

This comment has been minimized.

@oncletom

oncletom Dec 3, 2018

Collaborator

I encountered timeouts with large HTML pages. Better to make the server crash.

src: path.join(__dirname, '..', 'examples', 'document', 'asciidoctor.css'),
type: 'text/css',
},
'/asciidoctor-document.css': {

This comment has been minimized.

@Mogztter

Mogztter Dec 3, 2018

Owner

It should be /document.css to be consistent with <link href="document.css" rel="stylesheet"> right ?

This comment has been minimized.

@oncletom

oncletom Dec 3, 2018

Collaborator

Yes.

I was wary of naming collisions at this point and never completed the thinking around it.

Also, it made me think of the stylesheet and linkcss well known Asciidoctor attributes.

I'm happy to change either of these in a way which is consistent with Asciidoctor behaviour.

<script src="../../node_modules/pagedjs/dist/paged.polyfill.js"></script>
<link href="asciidoctor.css" rel="stylesheet">
<link href="document.css" rel="stylesheet">
<script src="paged.polyfill.js"></script>

This comment has been minimized.

@Mogztter

Mogztter Dec 7, 2018

Owner

I agree that using relative path can be confusing but I would prefer to introduce this change in an other pull request. We might as well decide to introduce a function to resolve ressources paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment