Skip to content

Conversation

@zbjornson
Copy link
Collaborator

Oddly, #646 seems to have broken browser tests (presumably on Windows only). I bisected the history back to that commit, although I don't see why it would have broken it.

Without this, the tests appear to not complete -- the empty onError hooks were suppressing a cairo error ("failed to read from input stream").

@LinusU
Copy link
Collaborator

LinusU commented Dec 7, 2015

Would it be better to solve this using path.join or path.resolve?

@LinusU
Copy link
Collaborator

LinusU commented Dec 7, 2015

Also, I don't see the empty onError hooks in your link, did you maybe misslink?

@zbjornson
Copy link
Collaborator Author

Oops, misslinked. Edited original post.

@zbjornson
Copy link
Collaborator Author

Just played a bit, but path.resolve doesn't appear to fix incorrect path seps. __dirname seems to have unescaped \, which is the problem.

@zbjornson
Copy link
Collaborator Author

@LinusU your test cleanup PR reminded me of this. I didn't explain this problem too well: on Windows, __dirname is something like C:\\foo\\bar\\node-canvas, and after the normalization in server.js the test functions contain paths like C:\foo\bar\node-canvas/public/star.png (when printed with if (/star/.test(req.body.fn)) console.log(req.body.fn);).

These work:

  • .replace(/\\/g, "/")
  • .replace(/\\/g, "\\\\")

These do not:

  • path.join(__dirname, "public", "star.png")
  • path.posix.join(__dirname, "public", "star.png")
  • path.win32.join(__dirname, "public", "star.png")
  • path.resolve(__dirname, "public/star.png")

which is to say, path.join and resolve won't touch the separators in __dirname.

@LinusU
Copy link
Collaborator

LinusU commented May 30, 2016

Ahh, I did not see the eval below, that explains it...

I'll take a look at this when I get home from work, sorry for the delay

@LinusU
Copy link
Collaborator

LinusU commented Oct 29, 2016

I fixed this another way in #830

@LinusU LinusU closed this Oct 29, 2016
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.

2 participants