Windows separator fix #41
Conversation
|
you'd have to adjust line 118 as well: result += JSON.stringify name.replace /\\/g,'/'plus: one could use the normalize function to get the separator: sep = normalize '/' |
|
@robofish Just addressed your first concern. It's now part of this pull request. As for |
|
cool :) Yeah, I'm wasn't too sure about |
| @@ -113,7 +116,7 @@ exports.Package = class Package | |||
| index = 0 | |||
| for name, {filename, source} of sources | |||
| result += if index++ is 0 then "" else ", " | |||
| result += JSON.stringify name | |||
| result += if isWindows then JSON.stringify(name.replace(/\\/g, '/')) else name | |||
There was a problem hiding this comment.
uh wait - you'll need another JSON.stringify for the posix case here
There was a problem hiding this comment.
Is that really what we want to do here? When on Unix, the path separators will already correctly be '/'.
There was a problem hiding this comment.
yeah, but the the point of this change is windows compatability, right? ;)
and I do have first hand knowledge that the packages will have \ in their path name, while the require statements use / as path separator => require won't find the package.
my point was that your latest change removes the 'JSON.stringify' from the name (which basically just adds the quotes) in case it's an unix system. you might want to do something like JSON.stringify( if isWindows then name.replace(/\/g, '/') else name )
There was a problem hiding this comment.
and I do have first hand knowledge that the packages will have \ in their path name
Ah, ok. I actually think this is a mistake.
"Windows compatibility" means you should be able to run Node on Windows, install the Stitch module, and everything should still work. It does not mean, for example, require paths should suddenly start using '' in their paths. This would tie your code to the system it's developed on, making collaboration nearly impossible. Requiring that stitch uses Unix paths parallels the uniformity of URL paths.
That said, if Node accepts both forward and back slashes for require for paths, stitch should also. Though it should accept both forms from all operating systems like Node and not show different error messages that depend on what OS Node happens to be running on.
There was a problem hiding this comment.
Just updated the pull request with better path normalization. Also, both path styles can now be used in require like with Node. See 2e4e6cd.
* Paths are now properly normalized upfront so both Windows-style and Unix-style paths are supported to behave like Node's require
* Module names are now always Unix-style, even when Windows-style names are used when calling "require"
* Consequently, error messages are always shown in Unix-style, which will encourage OS-independent code
e.g. "require('lib\\missing')" displays "'lib/missing' not found" regardless of the underlying OS
|
@sstephenson do you think you'll be able to merge this in soon? |
|
+1 |
Replaced the in-place separator string with a variable that gets set similar to the way Node does it.
When upgrading Node to a version that includes this commit, the declaration of
sepcan easily be replaced with the following change to line 5.{extname, join, normalize, sep} = require 'path'