search

bpmn-io / bpmn-to-image

Convert BPMN 2.0 diagrams to PDF documents or PNG files.
62 stars 38 forks source link

fix width when running inside docker #11

Closed KaiqueBressi closed 5 years ago

KaiqueBressi commented 5 years ago

Yesterday I opened an issue: https://github.com/bpmn-io/bpmn-to-image/issues/10

It's about an error that ocurrs just inside Docker Containers and happens with specific .bpmn. Depending on the bpmn file the width returning from desiredViewport is not integer, it's a float, causing the puppeteer to raise an exception.

I dont really know why it happens but the solution was to round the returned value from desiredViewport

pinussilvestrus commented 5 years ago

Hi @KaiqueBressi, really thanks for your contribution! The changes looks reasonable to me (as you described in #11). However, you can do two more things to get the Pull Request ready to merge:

  1. You can have a look at our Contribution guidelines. It would be nice if you can squash your two commits into a single one, which fits our commit message guideline. So like
fix(print-diagram): enforce integer value

Closes #11

It helps us to keep our commit history clean.

  1. Could you add a simple test case to verify the error will not occur again? Especially when this occurs just for certain diagrams (as you described), it would be great that we cover this inside our test suite. In the test case you can use a similar diagram file as you use and which leads to the broken behavior.

Feel free to contact us when you need help in any of this things 😊

KaiqueBressi commented 5 years ago

@pinussilvestrus It's very hard to reproduce this problem, i dont really know how to test it. This problem only happens inside a docker container with an specific bpmn file, i will try to investigate more to see if i find out the root of this error, maybe enforcing the integer value wont be necessary.

nikku commented 5 years ago

@pinussilvestrus Let's just clean up the PR and merge it without tests then. Not exactly sure what's happening inside a docker environment.

pinussilvestrus commented 5 years ago

Sure, I'll do so.

@KaiqueBressi can you maybe adopt the commit message to the form I proposed in this comment?

KaiqueBressi commented 5 years ago

@nikku and @pinussilvestrus Thanks for the reply. After investigating i discovered that the problem happens because of the line 71 of skeleton.html: 

bpmnViewer.get('canvas').zoom('fit-viewport');

After checking out the diagram-js there are some calculation of the width on the zoom function that may happen to return float instead of integer.

Anyway I fixed the commit message to fit the guideline. Thanks for everything.