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

Add Dash Apps Preview + PDF Generation #60

Closed
wants to merge 23 commits into from
Closed

Conversation

tarzzz
Copy link
Contributor

@tarzzz tarzzz commented Mar 15, 2018

} else {
// Default
result.format = 'png'
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to support other image types, since these are only going to be used for PDF exports and Organize thumbnails ..

win = null
})

contents.on('page-favicon-updated', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a placeholder for now .. !! It will be replaced with a custom event fired by Dash App once the rendering has finished.


contents.on('page-favicon-updated', () => {
if (PRINT_TO_PDF) {
contents.printToPDF({}, (err, pdfData) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO Error handling.. !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the PDF Options is set to {} until we have some more info from: https://github.com/plotly/streambed/issues/10698#issuecomment-373161617 ..

Copy link
Member

@chriddyp chriddyp Mar 15, 2018

Choose a reason for hiding this comment

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

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 15, 2018

@etpinard Can I please get an initial review on this.. I'll be adding some tests in the meantime.. !!

Update: Added error-handling in printToPDF call and tests.. !!

url: 'dummy',
format: 'pdf'
}, {}, (errorCode, result) => {
t.ok(win.webContents.printToPDF.calledOnce)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails because printToPDF waits for page-favicon-updated event before being called. Is there a way to handle this? The next tests also fail for similar reason.. !!

Copy link
Contributor

Choose a reason for hiding this comment

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

win is an event emitter as created here, so calling win.emit('page-favicon-updated') should allow you to test the callback.

})
} else {
contents.capturePage(img => {
result.imgData = img.toPNG()
Copy link
Member

@chriddyp chriddyp Mar 15, 2018

Choose a reason for hiding this comment

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

interesting, i'm curious what a PNG looks like! this could be very cool for thumbnails and feedviews 😸

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the plan. I did test it with the vanguard app:

a

It only captures the visible area in the browser, hence the need for width/height options ..

}

result.width = body.width || 800
result.height = body.height || 600
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting, I didn't think about this. I'm assuming that with PDF reports, where the content has absolute pixel sizing (e.g. the pages are 8.5" wide and 11" long), it doesn't matter what width and height we provide. However, for responsive apps, we'll need to provide something reasonable. Let's try a medium sized laptop: width: 1680px, height: 1050

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These options are only meant for png generation, as the printToPDF outputs are determined by: https://github.com/electron/electron/blob/master/docs/api/web-contents.md#contentsprinttopdfoptions-callback .. and not be the browser-size.

Although, we can add them in printToPDF options after some conversion as printToPDF expects height/width in microns.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I understand now. OK, lets just use the electron options directly instead of doing a pixel to micron conversion.

@etpinard
Copy link
Contributor

Looking good so far. Glad someone else is trying to add things to image-expoter. That was the goal all along.

Now, by preview you mean combining reports and thumbnail generation in one component? Electron's capturePage and printToPDF work very differently and support different options. So, I'd vote for splitting this thing into two separate components.

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 16, 2018

Now, by preview you mean combining reports and thumbnail generation in one component?

Yes, that was the my initial plan..

So, I'd vote for splitting this thing into two separate components.

In that case, I will remove the capturePage for now (if it seems ok to you), since we only need PDF generation for now (right @chriddyp ? ). I can add thumbnails after 2.4.0 release.

@chriddyp
Copy link
Member

So, I'd vote for splitting this thing into two separate components.

In that case, I will remove the capturePage for now (if it seems ok to you), since we only need PDF generation for now

yeah, we only need PDF generation for now

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 26, 2018

@etpinard @chriddyp: Some comments/updates:

  • we are checking for presence of waitfor div in dash-apps to determine if the app has finished rendering (before taking snapshot)
  • printToPDF's pdfOptions will be present in the request body and supplied to printToPDF.
  • Removed thumbnail-generation code from the PR..

@etpinard Please review..
The node (V8) tests seem to be failing for reasons unrelated to this PR.. would be great if you could take a look at them.. !!

Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

Great stuff @tarzzz !

I made a few comments. Most of them should be pretty trivial to fix. Regarding the node@v8 failures: it seems to be related to our is-url dependency. Using is-url@1.2.3 (the latest) npm test fails locally for me as on CI whereas using 1.2.2 (our package.json has "is-url": "^1.2.2") npm test our tests pass just fine.

One more thing: would you mind running npm run coverage locally (sorry I haven't hooked in https://coveralls.io/ yet) to see if all code branches if plotly-dash-preview are tested.

const result = {}

result.head = {}
result.head['Content-Type'] = cst.contentFormat['pdf']
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 cst.contentFormat.pdf would suffice.

module.exports = {
name: 'plotly-dash-preview',
ping: require('../../util/generic-ping'),
inject: plotlyGraph.inject,
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for an inject method here as the render step has no external deps.

sendToRenderer(code, result)
}

if (isNonEmptyString(body.url)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably be more strict here. Using isUrl (like done here) would be best.

return errorOut(400, 'invalid url')
}

result.pdfOptions = body.pdf_options || {}
Copy link
Contributor

Choose a reason for hiding this comment

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

What does Electron do when we pass invalid pdf options to printToPDF?

  • Does it ignore bad values and use the defaults?
  • Does it error out?

if the latter, we should try to intercept bad pdf options here, so that we return a 400-series parse error instead of 500-series render error.

Copy link
Contributor

Choose a reason for hiding this comment

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

... if the former, then we don't need to do anything here 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it ignore bad values and use the defaults?

That's correct. No 500s ..

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's great. We won't have to worry about the various printToPDF options 👌

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be less frustrating to the user if we could return a 400-series error (and have that bubble up through streambed). For example if a user is trying to set the page size they may try 'page_size': 'legal', get back a letter option (because that's the default), and keep trying variations of legal. If we instead sent a 400 error saying that page_size is an invalid option, that will be clearer to the user.

If this is a lot of work, I don't feel strongly about it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Returning a 400-series error here would be best from the user's standpoint. But since we would have to duplicate how Electron validates input here in the parse step in order to get this right, this may turn into a developer's nightmare for little gain. I'd vote for leaving this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, if it's easy let's do it but I agree that this isn't worth it if it's painful or a lot of work.

{
name: 'plotly-dash-preview',
route: '/dash-preview',
options: plotlyJsOpts
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to pass plotly.js options here. You can 🔪 this line.

function render (info, opts, sendToMain) {
const result = {}

let win = remote.createBrowserWindow()
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to specify the window width/height? That is, you're assuming all dash reports will have the same dimensions?

Copy link
Member

Choose a reason for hiding this comment

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

Some discussion on this earlier here: #60 (comment)

Now that I think about it, I wonder if we should still fix the size of the window before specifying the size of the PDF export. The PDF Export size will likely change the viewport (so responsive CSS will be reflected in the PDF) but I doubt that it will re-renderer all of the javascript. So, if there are certain components that are responsive with JS and not CSS, then I doubt that they will be reflected at the new size.

So, I think it makes sense to set size of this window to be the same size as requested in the PDF renderer. For printed content, 1px will be 1/96th of an inch (https://www.w3.org/Style/Examples/007/units.en.html). So 8.5 x 11 is 816px x 1056

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the browserwindow size be fixed at 816px x 1056 or determined on the basis of the print options Letter/A4 etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Elaborating on ⬆️ my point was that:

A4 paper measures in at 210mm by 297mm (8.26" by 11.69") whereas US Letter paper measures at 8.5" by 11" (215.9mm by 279.4mm)

Since difference is not too much between A4 and Letter, it shouldn't really affect the viewports if we change it by the paperSize (though we do get a significant difference when we consider A3/A2 as well)

If we consider A2, it comes out to ~1584.0px x 2246px .. which is big compared to general screen-sizes. (or xvfb screen sizes in case of headless server)

Copy link
Member

@chriddyp chriddyp Mar 26, 2018

Choose a reason for hiding this comment

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

Yeah, let's just be complete and compute all of them. From electron's docs:

pageSize String - (optional) Specify page size of the generated PDF. Can be A3, A4, A5, Legal, Letter, Tabloid or an Object containing height and width in microns.

const pixels_in_inch = 96;
const microns_in_inch = 25400;
const pixels_in_micron = pixels_in_inch / microns_in_inch;
const size_mapping = {
    'A3': {'width': 11.7 * pixels_in_inch, 'height': 16.5 * pixels_in_inch},
    'A4': {'width': 8.3 * pixels_in_inch, 'height': 11.7 * pixels_in_inch},
    'Letter': {'width': 8.5 * pixels_in_inch, 'height': 11 * pixels_in_inch},
    'Legal': {'width': 8.5 * pixels_in_inch, 'height': 14 * pixels_in_inch},
    'Legal': {'width': 8.5 * pixels_in_inch, 'height': 14 * pixels_in_inch},
    'Tabloid': {'width': 11 * pixels_in_inch, 'height': 17 * pixels_in_inch},
}
let browser_size;
let page_size = info.size;
if (Object.keys(size_mapping).indexOf(info.size) > 1) {
    browser_size = size_mapping[info.size];
} else if (typeof info.size.width !== 'Undefined' && typeof info.size.height !== 'Undefined') {
    browser_size = {'width': info.size.width * pixels_in_micron, 'height': info.size.height * pixels_in_micron}
} else {
    throw new Error('size must either be A3, A4, A5, Legal, Letter, Tabloid or an Object containing height and width in microns.')
}

}
})
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

What if finishedLoaded doesn't resolve?

win = null
})

let intervalId = setInterval(() => {
Copy link
Contributor

@etpinard etpinard Mar 26, 2018

Choose a reason for hiding this comment

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

I'm curious. Why did you decide to place the setInterval in the main window scope instead of in the child window scope?

Would something like:

const loaded = () => {
  return contents.executeJavaScript(`
     new Promise((resolve, reject) => {
       let tries = 10
       let interval = setInterval(() => {
         const el = document.getElementById('waitfor')
         if (el) {
           clearInterval(interval)
           resolve(true)
         }
         if (--tries === 0) reject('fail to load')
       }, 500)
     })
  `) 
}

loaded().then(() => {
   contents.printToPDF(info.pdfOptios, (err, pdfData) => {
      if (err) {
        done(525)
      } else {
        result.imgData = pdfData
        done()
      }
   }).catch(() => {
     done(525)
   })
})

be ok?

t.test('should handle printToPDF errors', t => {
const win = createMockWindow()
sinon.stub(remote, 'createBrowserWindow').returns(win)
win.webContents.executeJavaScript.resolves(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests! Like mentioned in an earlier comment, we'll need to handle (and test 😏 ) the case where executeJavascript does not resolve.

const finishedLoading = () => {
return win.webContents.executeJavaScript(`
new Promise((resolve, reject) => {
var a = document.getElementById("waitfor");
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a variable? i.e. part of the API where the use can pass in an arbitrary CSS selector. That would modify this to be document.querySelector.

For example, if the app has a graph in it, I might end up using a query selector like #mygraph .plot-container in order to wait until the graph was rendered (as plotly.js draws .plot-container dynamically). But, if the report doesn't have a graph in it, then I'd inject my own div with id waitfor

Copy link
Contributor Author

@tarzzz tarzzz Mar 26, 2018

Choose a reason for hiding this comment

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

Just to confirm, the query selector comes as the part of the incoming POST request (with .plot-container as default if nothing is provided)?

It should be safe to assume the dash-app has atleast one chart even if the incoming request does not specify it.

Copy link
Member

Choose a reason for hiding this comment

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

That's right - it will become part of the API, POST request. Either css-selector or timeout must be provided, no default values.

* Add setInterval to remote window instead of parent
* Check rendering by a timeout, or a css-selector (or both)
* Atleast one of “timeout” or “css-selector” must be provided in
request.
* Added catch for ‘executeJavascript’
Size can be specified by:
* pdf_options: pageSize key (“Letter”, “A3”, “A4” etc.)
* pageSize in request data
@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 27, 2018

@etpinard @chriddyp Updates made:

  • Atleast one of loading_selector or timeout should be specified in request. If timeout is specified, we wait for this time before capturing the page.
  • pageSize should be specified either as the part of the request body or as the part of pdfOptions. We use it to determine the browser window size as indicated by Chris in Add Dash Apps Preview + PDF Generation #60 (comment)
  • Added tests for these ⬆️ options as well as to check for rejected promises (executeJavascript.
  • Addressed minor code-style related comments.

Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

This is starting to look pretty solid 👌

@@ -24,6 +24,13 @@ function parse (body, opts, sendToRenderer) {
}

result.pdfOptions = body.pdf_options || {}
if (!body.loading_selector && !body.timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use utils isPositiveNumber for timeout and isNonEmptyString for loading_selector.

Oh and I would vote for just selector instead of loading_selector which is unnecessarily verbose here I think.

return errorOut(400, 'invalid url')
}

result.pdfOptions = body.pdf_options || {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's great. We won't have to worry about the various printToPDF options 👌

let tries = ${cst.maxRenderingTries}

if (${info.timeOut}) {
tries = parseInt(${info.timeOut} * 1000/${cst.minInterval})
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the coercion step to parse. Note that Number(info.timeOut) would suffice. parseInt is slow.

Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 1000 / ${cst.minInterval) please.

}

let interval = setInterval(() => {
let el = document.querySelector("${info.loadingSelector}")
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 use ' over " even inside ``

}
})
}, 500)
}).catch(() => {
done(525)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to not having noticed this before. But, it would be nice to separate printToPDF errors (that's the done(525) inside the printToPDF callback) and this one which is the result of a timeout.

I propose making 525 the printToPDF callback error, and adding 526 when we hit a timeout.

Copy link
Contributor Author

@tarzzz tarzzz Mar 28, 2018

Choose a reason for hiding this comment

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

526: 'plotly.js version 1.11.0 or up required',

526 is already taken.. maybe 408 Request Timeout or 504 Gateway Timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already taken in the plotly-graph component, right? Not in plotly-dash-preview? I don't like overriding 504 (a gateway timeout is something else) and I want this to be a 500-series.

package.json Outdated
@@ -49,7 +49,7 @@
"get-stdin": "^5.0.1",
"glob": "^7.1.2",
"is-plain-obj": "^1.1.0",
"is-url": "^1.2.2",
"is-url": "1.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok yeah. That might work. But really, we should be using the latest version of is-url. Looking at their changelog, bumping to version 1.2.4 should fix everything.

} else {
return errorOut(400, 'pageSize must either be A3, A4, A5, Legal, Letter, ' +
'Tabloid or an Object containing height and width ' +
'in microns.')
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 I really see indents like this in JS. That seems very pythonic. I would write this as:

errorOut(
  400,
  'pageSize must either be A3, A4, A5, Legal, Letter, ' +
  'Tabloid or an Object containing height and width ' +
  'in microns.'
)

@@ -31,6 +32,19 @@ function parse (body, opts, sendToRenderer) {
result.loadingSelector = body.loading_selector
result.timeOut = body.timeout

if (cst.sizeMapping[result.pdfOptions.pageSize]) {
result.browserSize = cst.sizeMapping[result.pdfOptions.pageSize]
} else if (body.pageSize && body.pageSize.width && body.pageSize.height) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use isPositiveNumeric(body.pageSize.width) and isPositiveNumeric(body.pageSize.height) to error early on 0 and negative inputs.

})

t.test('should error when pageSize is not given', t => {

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -23,6 +24,28 @@ tap.test('parse:', t => {
t.end()
})

t.test('should error when neither loading_selector or timeout is given', t => {

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!body.loading_selector && !body.timeout) {
return errorOut(400, 'either loading_selector or timeout must be specified')
if (!isNonEmptyString(body.selector) && !isPositiveNumeric(body.timeout)) {
return errorOut(400, 'either selector or timeout must be specified')
Copy link
Contributor

Choose a reason for hiding this comment

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

This here may be overly stringent. Perhaps we could default to a 3000ms timeout if timeout and selector are omitted from the POST request. @chriddyp what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer explicit over implicit in this case but I don't have a strong opinion.

if (${info.timeOut}) {
tries = parseInt(${info.timeOut} * 1000/${cst.minInterval})
}
let tries = info.tries || ${cst.maxRenderingTries}
Copy link
Contributor

Choose a reason for hiding this comment

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

Very clean here. Nice 👌

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 29, 2018

Added integration test, and ran coverage:
image

@tarzzz tarzzz force-pushed the add-dash-apps-preview branch 2 times, most recently from 350d91c to b7f4d18 Compare March 29, 2018 07:40
@tarzzz tarzzz force-pushed the add-dash-apps-preview branch 2 times, most recently from 0dcf16d to f326da5 Compare March 29, 2018 08:04
@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 29, 2018

Updates: Added integration test, and timeout code (526) for rendering related timeouts.

Still blocked by intermittently failing integration tests..

@@ -23,7 +23,7 @@ function render (info, opts, sendToMain) {
win.close()

if (errorCode) {
result.msg = 'dash preview generation failed'
result.msg = cst.statusMsg[errorCode]
Copy link
Contributor

Choose a reason for hiding this comment

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

nicely done 👍

@etpinard
Copy link
Contributor

@tarzzz thanks for running npm run coverage. You can ignore the uncovered lines in plotly-dash-preview/convert.js. But it would be nice to bring up plotly-dash-preview/parse.js to 100% coverage.

As for our plotly-export-server integration tests. Perhaps we should skip them on CI for now (and open a new issue about it). I think booting up 6 browser windows on the CI machines might be a little too much for them to handle. We could revisit this later.

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 29, 2018

@etpinard : Coverage ..

image

Added issue to address intermittent errors: #61

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 29, 2018

Closing in favor of #62

@tarzzz tarzzz closed this Mar 29, 2018
@etpinard etpinard deleted the add-dash-apps-preview branch April 18, 2018 14:15
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

Successfully merging this pull request may close these issues.

4 participants