Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Handle piping html directly via stdin #15

Closed
wants to merge 5 commits into from
Closed

Handle piping html directly via stdin #15

wants to merge 5 commits into from

Conversation

bsssshhhhhhh
Copy link
Contributor

Patch for #13

Passing - instead of a file or URL will now read from stdin

Using temporary files seems to be necessary here - from what I can see, Electron's browser window does not have an API for loading arbitrary HTML from a string.

Also, the docker run command requires the -i flag for this to work, otherwise it will produce a blank PDF output.

@MrSaints
Copy link
Collaborator

MrSaints commented Apr 18, 2016

Apologies. I may have caused some merge conflicts, but thank you for the PR 👍

I'm reasonably OK with the changes, although I'd prefer if we avoid external dependencies. I'm not too fuss with a little extra code maintenance vs. two external dependencies which also have their own dependencies just for a reasonably simple feature.

Also, I know that we use readFileSync twice in the codebase, but I'd prefer if we can reduce the number of synchronous calls in a Node app. I understand this might not be straightforward because of the way Node handles standard streams (and because of the current state of the code). It might also require a little bit of refactoring (that is, potentially wrapping app.on("ready" ... in a function).

I'm also wondering if we can skip the "temporary file" entirely, and just load the HTML through something like: https://developer.mozilla.org/en-US/docs/Web/HTTP/data_URIs

Nevertheless, I'd love to know what you think.

@bsssshhhhhhh
Copy link
Contributor Author

The synchronous calls are there only because this method changes the uriArg variable and needs it to be changed to a file path before the next if block. The rw module was included because Node produces unexpected results with readFile or readFileSync on non-regular files like /dev/stdin. Sometimes it works, sometimes not. The Node devs are unable or unwilling to fix it, so someone wrote a wrapper around the FileSystem API that handles non-regular files like /dev/stdin correctly.

The temp dependency was partly because of my laziness. You could generate a random filename in /tmp/ and the results would be the same, as long as you remember to delete the temporary file after athena is done with it. The temp module tracks all the temp files it creates and deletes them when the process exits, which is why I used it in this PR. No need to reinvent the wheel right?

As for reducing the number of synchronous calls in a Node app: As long as this is a command-line tool and not a server/listener application where the event loop needs to remain unblocked, I personally don't think it's that important. Refactoring working code just to do it 'the proper NodeJS way' may be more trouble than it's worth.

@bsssshhhhhhh
Copy link
Contributor Author

I fixed the merge conflict 👍

@bsssshhhhhhh
Copy link
Contributor Author

Closing this PR because I'm making another one

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants