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

Provide embedded http server #22

Closed
wants to merge 4 commits into from

Conversation

@oncletom
Copy link
Collaborator

@oncletom 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 and others added 3 commits Nov 26, 2018
Paged.js will transform an HTML5 page into a set of printable pages.
Margin, footer and header can be configured on each pages.
@Mogztter
Copy link
Owner

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

@oncletom 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
Copy link
Contributor

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

@oncletom 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
Copy link
Contributor

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

@oncletom 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
Copy link
Owner

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

@oncletom 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')
Copy link
Collaborator Author

@oncletom oncletom Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

@oncletom oncletom Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Collaborator Author

@oncletom oncletom Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}`)
Copy link
Collaborator Author

@oncletom oncletom Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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': {
Copy link
Owner

@Mogztter Mogztter Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@oncletom oncletom Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Mogztter Mogztter Dec 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Mogztter
Copy link
Owner

@Mogztter Mogztter commented Jan 23, 2019

I'm still not sure about the embedded HTTP server. It has a few advantages but now that Paged.js is using XMLHttpRequest it's not mandatory.
Thoughts ?

@oncletom
Copy link
Collaborator Author

@oncletom oncletom commented Apr 19, 2019

I'm still not sure about the embedded HTTP server. It has a few advantages but now that Paged.js is using XMLHttpRequest it's not mandatory.

Agreed, it's not mandatory.

I see one use case it can be useful: when --watch (or an hypothetic --preview option) is enabled. You get a URL to check the document output as a prepring proofreading, with a chance to see what's wrong in the browser devtools.

Another one is if a document has a need for CORS resources. An HTTP server will be required.

At the moment this is achieved by peaking at the temporary HTML file. If that's more handy, then we can drop the internal HTTP server and favour the file for now.

@Mogztter
Copy link
Owner

@Mogztter Mogztter commented Apr 20, 2019

I see one use case it can be useful: when --watch (or an hypothetic --preview option) is enabled. You get a URL to check the document output as a prepring proofreading, with a chance to see what's wrong in the browser devtools.
Another one is if a document has a need for CORS resources. An HTTP server will be required.
At the moment this is achieved by peaking at the temporary HTML file. If that's more handy, then we can drop the internal HTTP server and favour the file for now.

Using an internal HTTP server definitely makes sense and in the end we might introduce one but I don't want to commit to it just yet.
The reason is that I want to make sure that the converter design is good enough. I'm still not sure about the separation of concerns when it comes to editing/designing a PDF. I'm also still wondering about how we should bundle things together to make sure that the ressources are properly resolved.

@oncletom
Copy link
Collaborator Author

@oncletom oncletom commented Apr 21, 2019

Agreed 👍

Mogztter added a commit that referenced this issue Oct 29, 2019
@Mogztter Mogztter closed this in ef62d54 Nov 1, 2019
Mogztter added a commit that referenced this issue Nov 1, 2019
resolves #22 use link element and replace default stylesheet
@oncletom oncletom deleted the feat/pagedjs branch Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants