Skip to content

Conversation

@Pomax
Copy link
Contributor

@Pomax Pomax commented Jun 27, 2017

Closes #118

This makes everything node-runnable again, obviating the need for OS-presupplied utilities. It adds npm-run-all as a script running convenience, and it adds shx as cross-platform utility runner for common unix utils such as cp and rm. Finally, it also adds cross-env to ensure that environment variables are set in a way that work in unix, linux, OSX, and Windows alike.

npm test shows all tests passing, with the same coverage.

@Pomax
Copy link
Contributor Author

Pomax commented Jun 27, 2017

@devongovett probably worth landing, to unblock Windows users (either by choice or because work demands it) since CI says it's all good =)

@devongovett
Copy link
Member

Not sure how I feel about this. Doesn't Windows have bash now? What about cygwin?

Admittedly my knowledge of windows is very limited, but Make is a tool that's designed exactly for this purpose. npm scripts are not.

@Pomax
Copy link
Contributor Author

Pomax commented Jul 18, 2017

This is exactly what npm scripts were designed for, too, and have the benefit of actually working on Windows, whereas make simply doesn't (unless you make people jump through painful hoops).

That said, the Linux Subsystem is still in beta, so you can't really rely on it. Cygwin is massively huge, and for node projects there isn't a single thing in Cygwin that isn't already covered by node/npm packages that have clean CLI invocations. There is literally no reason to have any kind of reliance on make and Makefile if you're writing a node project =)

This PR simply ports the instructions you had in the Makefile to a form that fits "the proper node way" of bootstrapping and using CLI-based task running without baking in anything that's platform specific and prevents the project from running on one or more operating systems.

@davelab6
Copy link

davelab6 commented Jul 18, 2017 via email

@Pomax
Copy link
Contributor Author

Pomax commented Sep 25, 2017

hm. bump?

@Pomax
Copy link
Contributor Author

Pomax commented Jan 18, 2018

bump, again? =)

@Pomax
Copy link
Contributor Author

Pomax commented Jun 4, 2018

rerebump =)

(feel free to close it, but it would still benefit loads of people to merge this without any change in build funcationality)

@Pomax
Copy link
Contributor Author

Pomax commented Jul 29, 2018

@devongovett this is still very much worth landing

If you no longer really work on fontkit, could I selfishly ask you just hit merge so that people who do still use it a lot in the font work can use this normally on windows, without affecting anyone on nx operating systems? It doesn't get in your or anyone else's way, but does make life much easier for an estimated 50% of the developer community.

@devongovett devongovett merged commit bdd50f6 into foliojs:master Jul 29, 2018
@davelab6
Copy link

davelab6 commented Jul 30, 2018 via email

@devongovett
Copy link
Member

Yeah sorry guys, I've been busy with other projects lately. Today I made an org https://github.com/foliojs and I'll be moving fontkit, textkit, pdfkit, and all related dependencies across the whole stack there so more contributors can be involved with reviewing and merging code. @diegomura of react-pdf is already helping out a bit, and I hope to bring others on as maintainers as well. Hopefully that will help with the long term support for these projects.

@Pomax Pomax deleted the cross-platform branch July 30, 2018 16:19
@Pomax
Copy link
Contributor Author

Pomax commented Jul 30, 2018

That's great to hear!

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