Skip to content
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 replacement paths that are absolute paths on Windows #13

Closed
wants to merge 3 commits into from
Closed

Fix replacement paths that are absolute paths on Windows #13

wants to merge 3 commits into from

Conversation

roncli
Copy link
Contributor

@roncli roncli commented Jul 25, 2014

There is a bug where absolute paths on Windows have their backslashes replaced as forward slashes. For relative paths, this is not a problem. However, especially with joeybaker/remapify, it is possible to have absolute paths passed into aliasify, and getting the backslashes replaced there is problematic.

This fixes the issue by testing for an absolute path on Windows, and then replacing backslashes with double backslashes, which is necessary since the filename is in quotes. All other paths have their backslashes replaced by forward slashes as currently happens today. This also adds a new test for absolute paths to ensure the change is working properly.

There is a bug where absolute paths have their backslashes replaced as forward slashes.  For relative paths, this is not a problem.  However, especially with joeybaker/remapify, it is possible to have absolute paths passed into aliasify, and getting the backslashes replaced there is problematic.

This fixes the issue by testing for an absolute path on Windows, and then replacing backslashes with double backslashes, which is necessary since the filename is in quotes.  All other paths have their backslashes replaced by forward slashes as currently happens today.
@jwalton
Copy link
Contributor

jwalton commented Jul 29, 2014

The thing I don't get here is why the "\"? The require we write is in single quotes, so wouldn't you just want ? I tried modifying your unit test:

it "should correctly resolve absolute paths", (done) ->
    jsFile = "c:\\foo.js"

    aliasifyWithConfig = aliasify.configure {
        aliases: {
            "foo": jsFile
        }
    }

    content = """
        foo = require("foo");
    """
    # Note the \\ here, because this is in "s, but this resolves to a single \.
    expectedContent = """
        foo = require('c:\\foo.js');
    """

    transformTools.runTransform aliasifyWithConfig, jsFile, {content}, (err, result) ->
        return done err if err
        assert.equal result, expectedContent
        done()

But this fails, because the end file has require('c:\\foo.js') instead of require('c:\foo.js').

@jwalton
Copy link
Contributor

jwalton commented Jul 29, 2014

Oh, also, a unit test like the one above that's windows specific would be awesome. :)

@roncli
Copy link
Contributor Author

roncli commented Aug 8, 2014

Single vs double quotes won't matter. You need a double backslash because the code outputs the path inside a string.

Your test case will fail because:

"""
foo = require('c:\\foo.js');
"""

will output as


foo = require('c:\foo.js');

And 'c:\foo.js' (and also "c:\foo.js") will evaluate as c:♀oo.js. To fix that test case, expectedContent should be declared with four backslashes instead of two.

Given this is nodejs, I did not want to write a test case specific to Windows. The test case I attached should cover all OSes, and shows that the issue is fixed on Windows while not introducing new issues on other OSes. However, if you still believe one specifically for Windows should be added, I can look into doing that this weekend.

@jwalton
Copy link
Contributor

jwalton commented Mar 6, 2015

Ooooh! I finally understand what you're saying about the quadruple backslash. I feel so dense. :P

@jwalton
Copy link
Contributor

jwalton commented Mar 6, 2015

Published as 1.7.0 - sorry this took so long. :)

@jwalton jwalton closed this Mar 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants