-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Comments
Yes, that's the plan. I think @jbovenschen might have started on it already? |
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 |
Cool @devongovett @jbovenschen! I will give it a try next week. |
@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. |
What node version it should support? I think v4 is good enough. |
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. |
@jbovenschen I see you have a branch coffee-to-babel on your fork. Should @thiamsantos start from that? |
Sure, no problem! |
He could continue on my branch, though in the end it would maybe be easier to first setup the About the semicolons thing is it an idea to use something like |
@thiamsantos How is converting |
@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. |
I started the conversion here. One question: I can take the liberty to make small refactors in internal code? |
@thiamsantos we can, but I'd rather do that in a follow up PR so we can make the diff easy to see. |
@thiamsantos, How are you doing with this task. |
@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. |
@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. |
@draganmarjanovic Will look into this issue again this weekend, will try to get my branch more up te date with the current master of |
@elpic worked on this, and I think he finished: https://github.com/elpic/pdfkit |
Yes. I got the part of doing separate builds. |
I think he probably meant fontkit: https://github.com/devongovett/fontkit. It uses rollup to build. |
@devongovett and is rollup shimming the node dependencies automatically with that config? |
@diegomura no you'd need to build your app using fontkit with something like browserify or webpack to do that. |
Yes I meant, the fontkit variant. For 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. |
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. |
Any progress on this? @jbovenschen is the last thing left to do is the roll up bundling? |
Would microbundle be useful for this? |
Hello) I'm trying use pdfkit-es repo in browser and get error with afm and readFileSync in browser |
This should be closed, though? |
@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?
The text was updated successfully, but these errors were encountered: