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
Fix external relative paths on Windows #1704
Conversation
Replace backslashes with forward slashes when adding relative paths for external references.
Fixes the following scenario:
|
Build is failing for the same reason that the master branch is failing https://travis-ci.org/substack/node-browserify/builds/207173302 |
grunt-test.zip |
...I may have just had a misunderstanding of how Browserify works. I will have to verify tomorrow, but it seems like my change just gave me the illusion of fixing the problem, when in reality it was just including |
Can confirm. All my change seemed to do was invalidate the |
Firstly, ignore my previous grunt-test.zip. It was more complicated than necessary. grunt-test.zip dist-before.zip dist-after.zip So while I think there is a problem in this use case, my changes didn't fix it. |
There's no doubt there is a problem (as the numerous issues attest). What I'm afraid of that there's not any incentive to fix it (there were multiple PRs somewhat addressing windows backslash issues, some still open). /cc @substack should we attempt to issue a PR to fix this at all? |
Ignore node_modules directory
Fixed it properly this time by replacing all \ with / in relative paths. Now it works as expected. Fun fact: my change decreased the number of failing tests on my machine from 61 to 53. Again, Travis check is failing for the same reason as the master branch: ArrayBuffer is not defined in Node 0.10.x. |
Use the same arguments as the function exported by cached-path-relative, rather than calling .apply
Is there anything else I can do to help get this merged in? |
I am also facing same issue. Will this get merged? |
@kauvj I haven't heard anything, but I merged in the current master branch to get this to build. |
@@ -118,14 +119,12 @@ Browserify.prototype.require = function (file, opts) { | |||
var expose = opts.expose; | |||
if (file === expose && /^[\.]/.test(expose)) { | |||
expose = '/' + relativePath(basedir, expose); | |||
expose = expose.replace(/\\/g, '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you use your helper function from below here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. My changes basically extended the existing relativePath
method to replace backslashes with forward slashes, which means that adding the .replace(/\\/g, '/')
after a relativePath
call is no longer necessary.
I don't quite remember, but I think the problem was that .replace(/\\/g, '/')
was missing after some of the other relativePath
calls and so the path behaviour was inconsistent on Windows.
} | ||
if (expose === undefined && this._options.exposeAll) { | ||
expose = true; | ||
} | ||
if (expose === true) { | ||
expose = '/' + relativePath(basedir, file); | ||
expose = expose.replace(/\\/g, '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! this fixes some tests on windows too 🎉
Will merge this in #1819. Thanks! |
Awesome. Thanks! |
Replace backslashes with forward slashes when adding relative paths for external references.