search

danburzo / percollate

A command-line tool to turn web pages into readable PDF, EPUB, HTML, or Markdown docs.
https://danburzo.ro/projects/percollate/
MIT License
4.32k stars 166 forks source link

use fileURLToPath for windows compatibility #139

Closed XiangRongLin closed 2 years ago

XiangRongLin commented 2 years ago

This change makes it possible to generate epubs on windows by using a cross plattform compatible way to resolve the path.

See: https://github.com/nodejs/node/issues/37845

The same problem exists for the tests https://github.com/danburzo/percollate/blob/9ff43d022e37e2faf0297e02a213e26b98a78326/test/index.test.js#L30 and https://github.com/danburzo/percollate/blob/9ff43d022e37e2faf0297e02a213e26b98a78326/test/index.test.js#L35 but I couldn't get it to work. My attempt was by using fileURLToPath() in the same way as in this PR and replacing this line https://github.com/danburzo/percollate/blob/9ff43d022e37e2faf0297e02a213e26b98a78326/index.js#L154 with decodeURI(fileURLToPath(url)), but readFile() does not work with windows style paths, like C:\Users\something So I left that out of here.

danburzo commented 2 years ago

Thank you for this PR, @XiangRongLin! That line was indeed intended to be a replacement for __dirname and I wasn't aware pathname doesn't produce correct results on Windows. 😬

I will look into the other issues you pointed out.

XiangRongLin commented 2 years ago

I wasn't aware pathname doesn't produce correct results on Windows. 😬

You could maybe expand the CI workflow to also run the test suite on windows. I think windows runners are also for free

danburzo commented 2 years ago

Yes, that is definitely the way to go. I'll try to get a local environment to make the tests pass, then CI will work well to spot regressions.