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

Port to ES6 #707

Closed
thiamsantos opened this issue Jul 18, 2017 · 30 comments
Closed

Port to ES6 #707

thiamsantos opened this issue Jul 18, 2017 · 30 comments

Comments

@thiamsantos
Copy link

@devongovett I saw here and here that you want to port the Cofeescript code to ES6. That's why I would like to know if a pull request for this change would be welcome. I am willing to do it using decaffeinate. What are your thoughts?

@devongovett
Copy link
Member

Yes, that's the plan. I think @jbovenschen might have started on it already?

@jbovenschen
Copy link

I did some work on it, never really had the time to finish it yet because I was moving to a different city. If you want to try @thiamsantos it should be fine, most of the code base was possible to convert using decaffeinate, though certain files did not behave the same as before.

@thiamsantos
Copy link
Author

Cool @devongovett @jbovenschen! I will give it a try next week.

@thiamsantos
Copy link
Author

@devongovett @jbovenschen Do you have any preferences aboyt a code style? I was thinking in use xo with no semicolons and 2 spaces of indentation.

@thiamsantos
Copy link
Author

What node version it should support? I think v4 is good enough.

@devongovett
Copy link
Member

semicolons please! 😁

We do need to be careful about backward compatibility, so we will definitely need to run babel over the codebase prior to publishing. PDFKit currently supports node 0.10 and later. I was also thinking of using rollup to bundle everything up as well, similar to how I did in fontkit. See here for the config.

@devongovett
Copy link
Member

@jbovenschen I see you have a branch coffee-to-babel on your fork. Should @thiamsantos start from that?

@thiamsantos
Copy link
Author

semicolons please! 😁

Sure, no problem!

@jbovenschen
Copy link

He could continue on my branch, though in the end it would maybe be easier to first setup the rollup bundling and after that convert one file at a time to make it easier to verify if everything behaves the same way as before.

About the semicolons thing is it an idea to use something like prettier?

@thiamsantos
Copy link
Author

About the semicolons thing is it an idea to use something like prettier?

Yes, I want to use prettier along with xo.

@jbovenschen
Copy link

@thiamsantos How is converting pdfkit from coffee script to es6 going? Anything I can help you with?

@thiamsantos
Copy link
Author

@devongovett I will start today. Last week I was too busy. I was thinking in send a pull request to your fork, to continue the work already done.

@thiamsantos
Copy link
Author

I started the conversion here.

One question: I can take the liberty to make small refactors in internal code?

@devongovett
Copy link
Member

@thiamsantos we can, but I'd rather do that in a follow up PR so we can make the diff easy to see.

@mbarakaja
Copy link

@thiamsantos, How are you doing with this task.

@thiamsantos
Copy link
Author

thiamsantos commented Aug 23, 2017

@mbarakaja I'm in the last year of school, and unfortunately I don't have any free time right now to do this task. I think that someone should take this task.

Most of the work was done by @jbovenschen but lots of things are still missing.

@draganmarjanovic
Copy link

@thiamsantos Is there a roadmap / checklist? I'd be glad to help I'm just not sure where to pick it up from and what needs doing.

@jbovenschen
Copy link

@draganmarjanovic Will look into this issue again this weekend, will try to get my branch more up te date with the current master of pdfkit, and give you some pointers how to contribute to it, will try to make it able to convert pdfkit file by file instead of all at once, which seemed to be a lot of work.

@diegomura
Copy link
Collaborator

@elpic worked on this, and I think he finished: https://github.com/elpic/pdfkit
Worked on every scenario I tried.
My interest on seeing this working is to be able to finally see react-pdf working in the browser. I been trying to make it work, but without success. @jbovenschen @devongovett can you give me (if you know) some guidance on how you would do it?

@jbovenschen
Copy link

@elpic thanks for doing this!

What should be done is that we bundle the library the same way as done in svgkit.

After that is done we should make two separate build for node and web, in the web version we should provide shims for the node specific modules.

@diegomura
Copy link
Collaborator

diegomura commented Oct 16, 2017

Yes. I got the part of doing separate builds.
Seeing svgkit, I didn't get how it's being bundled. Can you be a bit more specific?

@devongovett
Copy link
Member

I think he probably meant fontkit: https://github.com/devongovett/fontkit. It uses rollup to build.

@diegomura
Copy link
Collaborator

@devongovett and is rollup shimming the node dependencies automatically with that config?

@devongovett
Copy link
Member

@diegomura no you'd need to build your app using fontkit with something like browserify or webpack to do that.

@jbovenschen
Copy link

Yes I meant, the fontkit variant. For web libraries it would be really handy if the bundle was already made... So you don't need to compile it later on.

Pointing to the web package can be done with a browser key in the package.json.

For the shimming of the node modules with rollup I tend to use this plugin, and if it is still big just write some wrappers based on env variables.

@devongovett
Copy link
Member

The problem with doing it this way is that if you require pdfkit with browserify or webpack, and another library you use also has node shims, you'll get duplicates in your application. I have provided prebuilt versions of pdfkit in the past including those shims by using browserify, but IMO the default should not include them for the above reason.

@okcoker
Copy link

okcoker commented Feb 8, 2018

Any progress on this? @jbovenschen is the last thing left to do is the roll up bundling?

@homerjam
Copy link

Would microbundle be useful for this?

@romanlex
Copy link

romanlex commented Nov 2, 2018

Hello) I'm trying use pdfkit-es repo in browser and get error with afm and readFileSync in browser
When we can get modern version of pdfkit with optimizations?)))

@fatso83
Copy link

fatso83 commented Nov 30, 2018

This should be closed, though?

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

No branches or pull requests

10 participants