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

Upgrade paged.js to version 0.2.0 #252

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

felipecrp
Copy link
Contributor

I upgraded the paged.js from version 0.1.43 to 0.2.0.

I noticed that we use the paged.polyfill.js instead of the paged.js file. Hence, I renamed the file to make future upgrades straightforward.

Regards,

@RLesur RLesur self-requested a review November 14, 2021 15:21
@RLesur RLesur self-assigned this Nov 14, 2021
Copy link
Collaborator

@RLesur RLesur left a comment

Choose a reason for hiding this comment

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

Hi @felipecrp,

Thanks a lot for this PR. We definitely should upgrade Paged.js to 0.2.0.

Usually, we upgrade Paged.js running the tools/update.R script. Sorry, this is undocumented. If we want to rename paged.js to paged.polyfill.js, we should update this script too.

I haven't upgraded Paged.js to 0.2.0 yet because it brings a native support for footnotes (see https://www.pagedjs.org/posts/2021-06-newrelease/) which is very cool.

However, this feature relies on a major change in the Paged.js page box model. From my experience, this kind of change can easilly break some templates. Before upgrading to 0.2.0, we should carefully test pagedown builtin templates (and maybe the pagedreport ones). Have you had time to test them?

While upgrading to Paged.js 0.2.0, I think we also should remove the footnotes support introduced by #21 and offer a new support for footnotes relying on Paged.js' native one. I can do it but I won't have the bandwith before december.

@felipecrp
Copy link
Contributor Author

Hi @RLesur, I did just minor tests with my documents. I agree that more tests are required.

I saw some unit tests on the project, but they apparently test some render features but do not assert whether pagedown created the output correctly. It would be nice to have automated regression tests, but I understand that this might be difficult because we need to compare the generated files visually.

We need to think about how to create some tests to enable regression before major changes. Do you have any suggestions?

Regards,

@RLesur
Copy link
Collaborator

RLesur commented Nov 14, 2021

@felipecrp Having the tools to perform visual tests against regressions would be great. The .test parameter introduced in 031b1cb and d6d649d was the very first step in this direction. We could compare the PDF files or pages screenshots. I did some attempts 2 years ago to compare pages screenshots and had serious issues while automating the process. Maybe comparing the PDF files would be an easier move.

@felipecrp
Copy link
Contributor Author

@RLesur some tests can also explore the generated DOM. For instance, a javascript code could check whether a header is repeated on every page or if a footnote is presented at the bottom of the page.

@felipecrp
Copy link
Contributor Author

felipecrp commented Nov 14, 2021

I just saw that the paged.js use this strategy (DOM check) for unit testing.

https://gitlab.pagedmedia.org/tools/pagedjs/tree/master/specs

They basically have an HTML and a javascript file that check whether the result is ok. I think we could do the same, but with RMarkdown.

@RLesur
Copy link
Collaborator

RLesur commented Nov 14, 2021

@felipecrp You're right, DOM checking could be another strategy. However, I wonder whether this would be useful in the context of pagedown. For instance, in the context of the current PR, DOM checks would warn that the DOM have changed (because the Paged.js' page box model changes), this wouldn't be a great help. But feel free to send a proposal!

@felipecrp
Copy link
Contributor Author

felipecrp commented Nov 14, 2021

@RLesur I think we only need to unit test the 'hacks' included on pagedown, e.g., the hooks or custom behaviors.

@cderv
Copy link
Collaborator

cderv commented Nov 15, 2021

Automated more tests would be awesome ! But testing R Markdown ecosystem is really not easy. Especially pagedown which acts at the client level with lots of JS in the browser.
FWIW paged.js is using JEST JS framework (https://jestjs.io/) for its testing, but it is not something easy for us to use directly in our ecosystem.
Anyway, this auto-testing topic of R Markdown output is something broader that I would like to tackle in the future so happy to take any idea and aspiration.
Putting here some idea that I believe we could need to use something like to stay in the R ecosystem

  1. Use testthat as a testing framework in R
  2. Use chromote or crrri for load the HTML rendered using chromium
  3. Dump the DOM in a new HTML file & use xml2 to parse and check the DOM expectations

We could also switch to a JS framework for that but it would need to run R in the first step and we would need to develop more JS skills. From paged.js test workflow:

  1. Use Jest or another framework to run tests in JS
  2. Run R from JS to render Rmd to HTML
  3. Use pupetteer to render the page in a browser for JS to work
  4. Use JS code within the jest framework to check the content of the rendered HTML

I don't know if not using R as advantage here for testing R package.

Last solution we already think about is a printing to PDF or image and doing a visual comparison but this has other challenges and if we detect an issue, it would need manual visual inspection for find the difference, then the part in the DOM, then the code that fail.

Anyway, interesting topic as you see! 😄

but after all , I think this would still not prevent us to do some manual visual checks of our template and pagedreport template. Not everything change can be auto test. What we could improve right now is make a checklist, and some helper functions so that we can easily run manual test without much trouble and know what to look into. I believe this would be the first step before

@felipecrp
Copy link
Contributor Author

I was trying to set up the test environment yesterday, using JEST but got some issues with my WSL installation to use puppeteer.

Since we need to test the DOM, I believe that it will be easier to use a JS test framework, such as JEST. Of course, we need to start from an Rmd file, then we need to use a workflow like the following:

  1. Parse Rmd and generate the HTML file
  2. Run the HTML file and assert the generated DOM

Maybe, the easier way to accomplish this is to use testthat just to generate the temporary HTML files and JEST to consume the temporary HTML, assert, and delete. We just need to assert that testthat would run first. Also, we could try to use some JEST before assignments to call R, but I think this could be more complicated.

PS: I think this discussion deserves another PR/issue because it is a much complex issue than the upgrade itself.

Regards,

@cderv
Copy link
Collaborator

cderv commented Nov 15, 2021

Since we need to test the DOM, I believe that it will be easier to use a JS test framework, such as JEST.

FWIW testing the DOM content using xml2 and XPATH request is what we do in other tools to test that the created DOM is the one expected. Also chromote or crrri would be enough to do like puppeteer from R directly.

PS: I think this discussion deserves another PR/issue because it is a much complex issue than the upgrade itself.

Agreed I share my thoughts here because I was under the impression you would have already tried something - and I was right 😉

Please feel free to open a new issue to share your idea, your progress and thoughts on all this. I don't know what the best solution would be. I just know this is not something pagedown specific and it is more broader problem. But trying something with pagedown is definitely worth it !

@felipecrp
Copy link
Contributor Author

Hi @cderv,

I just created the #253 to share my progress (still work in progress).

Regards,

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.

3 participants