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

Convert to EcmaScript (ES6+) #859

Merged
merged 40 commits into from
Nov 29, 2018
Merged

Conversation

blikblum
Copy link
Member

@blikblum blikblum commented Aug 19, 2018

Converts code base to modern JavaScript. The build targets to node 6+

It builds on top of #806 to ensure the converted code acts the same as before

It also incorporates changes from #729

A few changes were also done:

  • remove circular reference between PDFObject and PDFReference / PDFFont and its subclasses
  • Do not try to reset reference buffer length (did not work with recent node and also has no actual effect)

Todo:

  • Cleanup decaffeinate artifacts
  • Remove coffeescript build dependencies
  • Convert demos to ES6
  • Use rollup to create bundle
  • Add browser specific builds (use ES export, targets E11 and green browsers)

CorvusCorrax and others added 23 commits September 8, 2017 10:29
…s a pdf with the difference if snapshot does not matches
@devongovett
Copy link
Member

Awesome work! 👏

cc. @diegomura

@diegomura
Copy link
Collaborator

diegomura commented Aug 21, 2018

Great to see this moving forward!

@blikblum I did this on a fork when working on react-pdf. Never had time to polish it in order to make a PR (specially because it has some other changes), but wanted to share it in case it helps for something 😄

@rsshilli rsshilli mentioned this pull request Aug 24, 2018
@blikblum
Copy link
Member Author

This is ready to go and to broader testing.

To make easier testing, i uploaded to npm a build of this branch as pdfkit-es

Those that want to help can:

  • report difference of pdf rendering compared to latest published build
  • report perf regression
  • send sample code that uses gradient and outline API (to be incorporated as tests)

@blikblum blikblum changed the title [WIP] Convert to EcmaScript (ES6+) Convert to EcmaScript (ES6+) Aug 30, 2018
@blikblum
Copy link
Member Author

Users that are creating pdf with many png files can test the pdfkit-es@pngsync version.

Its a try to fix a perf issue: #861

@diegomura
Copy link
Collaborator

Hey @blikblum ! Awesome work. Would love to see this merged soon.

Did you have any feedback about how this version is working? I'm going to try finding time to test it also.

I find a bit confusing tests under the pdfmake dir. Where do they come from? I usually try to avoid monolitic tests for a more specific subset of them, but maybe I'm missing something.

@blikblum
Copy link
Member Author

Did you have any feedback about how this version is working? I'm going to try finding time to test it also.

It should be working exactly as the last coffee version + the memory leak PR (the test snapshots were created with this coffee version). But only one user reported a testing (successfully) so far.

I find a bit confusing tests under the pdfmake dir. Where do they come from?

They come from the pdfmake examples. I instrumented pdfkit to record the calls and generated the tests

I usually try to avoid monolitic tests for a more specific subset of them, but maybe I'm missing something.

Agree. This is the way i found to have the broadest tests to avoid regression. Since there was no previous test at all this seems a reasonable compromise. As the time goes and more tests are added they can be removed. To achieve that a good rule is to only accept patches with tests

@swftvsn
Copy link

swftvsn commented Oct 26, 2018

@devongovett @blikblum is there a reason why this hasn't been merged already?

One possibility would be to start developing pdfkit-es forward and leave pdfkit as is, projects could then adopt to that library when they are ready.

The current state of affairs really hinders the development, so I think something needs to happen soon.

Ps. I need PDF/A and would like to work on top of this so I could add tests etc. but am not sure if this will ever be merged given the time it has taken..

@atiertant
Copy link

@blikblum really good ! but why do we need coffee-script in dependencies ?

@blikblum
Copy link
Member Author

@blikblum really good ! but why do we need coffee-script in dependencies ?

AFAIR docs still uses coffee to be generated, something that can be easily cleaned after the merge

@liborm85
Copy link
Collaborator

@devongovett @alafr Can you merge this PR and release new version?

@devongovett
Copy link
Member

Hey @blikblum this is amazing, thanks so much for doing it! I’m sorry it has taken this look for me to look at it.

Going to merge, but I saw a couple things that might be good to follow up on:

  • There are still some decaffinate suggestions in some of the files. Would be good to check through those and fix/remove them.
  • Would be super awesome to enable prettier on the repo. There is some inconsistent code formatting in the files at the moment.

I’ve also added you as a maintainer of the repo. Thanks again for your contribution. 🙏

@devongovett devongovett merged commit 83f5f72 into foliojs:master Nov 29, 2018
@blikblum
Copy link
Member Author

Going to merge, but I saw a couple things that might be good to follow up on:

I'm aware of it but the PR was already too big. Will do the cleanup this weekend

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.

7 participants