Limit use of require("path") to resolve Windows compatibility issues

[roncli]

This pull request is to eliminate the use of require("path") in order to resolve issues with Windows.

First, the alias has been updated to not use path.join. Using path.join changed the path names from using forward slashes to backslashes on Windows, but kept them as forward slashes on Linux. Now, the alias will consistently use forward slashes.

Second, the file path has been updated to not resolve to an absolute path. Use of absolute paths was exposing the local file system's absolute path in the client source in browserify (rendr, for instance, would expose the location of the app/router file in the client-side mergedAssets.js file). While this can work, it is not ideal to expose the absolute path. Now, the user can choose whether to use a relative or absolute path based on the cwd option.

[joeybaker]

@roncli I think we're working at cross-purposes here. The point of path.join is to work cross-platform. If something is broken for you, can you add a failing test case?

[roncli]

The statement about the purpose of path.join is valid. However, this doesn't apply to the alias.

I installed mocha and ran the existing test cases. Two already fail under Windows:

  1) remapify exposes the files under a different alias:
     Uncaught AssertionError: expected { Object (path\a.js, path\a, ...) } to contain keys 'path/a.js', 'path/b.js', 'path/nested/a.js', and 'path/nested/c.js'

  2) remapify works without the expose option:
     Uncaught AssertionError: expected { Object (a.js, a, ...) } to contain keys 'a.js', 'b.js', 'nested/a.js', and 'nested/c.js'

What's happening is that the alias is getting its path localized to its platform. path/a remains path/a on Linux, but becomes path\a on Windows. A developer is unable to write a module based on this test that points to path/a without doing something like require('path' + path.sep + 'a').

As for the filePath, on both Linux and Windows it can expose the full path of a require file. This to me seems to be a security risk, so I made this optional. If the developer puts a relative path in cwd, the filePath is relative. If the developer puts an absolute path in cwd, then filePath is absolute.

FYI, there were a couple of bugs with my implementation that I found in running the tests that I didn't have in the project I'm using this in, so I'm going to test this a bit more thoroughly and redo this PR. I will also update the tests and add one specifically for the absolute cwd case. I'll take care of that later today.

[joeybaker]

@roncli thanks! I've re-opened this, so just push the updates to the same branch and let me know when ready.

[roncli]

@joeybaker I had a bit of a struggle with gulp. For much of the day, it was running the lint job but not the test job, even on commits I had passing before. I think there was a transient error in one of gulp's dependencies because it started working again this evening after I wiped node_modules and reinstalled. I tend to do that frequently while developing. :)

Anyway, good news is I got all 6 of the original tests and my one new test to pass on both Windows and on travis. roncli/remapify@14151dd is the latest commit, let me know if you have any further concerns, and thanks for your response!

[danielgutenberg]

I have also been having issues on Windows. @joeybaker I have tried using your branch, but I still get the following:

{ [Error: module "C:/xampp/htdocs/web/js/router.js" not found from "C:\\xampp\\htdocs\\web\\js\\app.js"],
  filename: 'C:/xampp/htdocs/web/js/router.js',
  parent: 'C:\\xampp\\htdocs\\web\\js\\app.js' 
}

Is this the same issue or should I open a new bug?

[roncli]

@danielgutenberg Yes, this is the same issue. It is complaining that your app.js is requiring "C:/xampp/htdocs/web/js/router.js", which is 1) an absolute path, and 2) using forward slashes.

[joeybaker]

Thanks guys. I'll review and merge soon!

[tleunen]

hi @roncli, I tested the code from your fork, and I still have an issue when resolving the dependancies. Instead of not finding the module with its name, I now have an error with absolute path:

>> Error: module "C:/xxxx/MyOtherModule.js" not found from "C:\\xxxx\\Mymodule.js"

Note that one path has / and the other \\. Is there anything else to do to make it work on windows?

[roncli]

@tleunen I looked into this, and it seems to be an issue with benbria/aliasify. When it replaces a require call, it's replacing the backslashes with forward slashes. This doesn't work for an absolute path. I'm going to look further into this.

A workaround would be to use a relative path in your cwd option. For instance, in my rendr app, I am using "./app" instead of "c:\\dev\\git\\github\\roncli.com\\roncli.com\\app".

[roncli]

I worked out the problem and have submitted benbria/aliasify#13 to deal with absolute paths on Windows.

[cnelson87]

Hello, and thank you for the fix. Will it be made public soon?

[roncli]

Hopefully! :) Ultimately, it's up to the developer.

Theoretically, this change could be applied. Absolute path functionality in Windows does not work today, and this patch does not fix it, as the problem resides in aliasify, a child module. I have PR benbria/aliasify#13 out with aliasify to fix the issue, and hopefully @jwalton will be able to look at it soon.

However, relative path functionality - the functionality that I would expect to be used in the majority of use cases - in Windows is resolved by this patch.

I am developing a rendr website on Windows and have been submitting a number of PR's with the goal of getting rendr working with the latest modules, so I, too, am looking forward to having these integrated!

[joeybaker]

I'm currently reviewing, sorry it's taken so long. Should be soon.

[joeybaker]

Okay, after playing with this some. I think the problem might actually be related to configuration vagueness/conflicts. Stay tuned…

[joeybaker]

This might/should be fixed by #8. Can you try version 1.0.0. Can you try it and confirm?

[cnelson87]

I updated to remapify 1.0.0 and now I'm getting an error in my grunt browserify task: Fatal error: Cannot read property '1' of null. I did not get this error with 0.1.6. I'm not really sure how to debug / troubleshoot. If you have any suggestions I would be willing to try.

[joeybaker]

@cnelson87 can you show me your remapify config?

[cnelson87]

Sorry for the delay, been busy with work. Here's an example of how I'm using grunt-browserify and remapify: https://github.com/cnelson87/grunt-browserify-214-test/blob/master/grunt_tasks/browserify.js

[joeybaker]

@cnelson87 I see. Can you try something for me? Instead of passing relative paths, pass absolute ones in cwd.

{
  cwd: path.join(__dirname, './src/scripts/config'),
  src: '**/*.*',
  expose: 'config'
},

Let me know if that works.

[cnelson87]

Note that I have my grunt tasks in a sub folder 'grunt_tasks' and load them via the 'load-grunt-tasks' plugin. The sub folder is included in '__dirname' and I had to strip it out before joining paths. Regardless, same error: Fatal error: Cannot read property '1' of null.

[joeybaker]

@cnelson87 and just to confirm, this doesn't work either?

{
  cwd: path.join(__dirname, '../src/scripts/config'),
  src: '**/*.*',
  expose: 'config'
},

[cnelson87]

That's much simpler than what I was doing, but no, sorry, still the same error.

[joeybaker]

Fair enough. I'll take a deeper look when I can spin up a windows VM.

[cnelson87]

To clarify, I get the error on both Mac and Win.

[joeybaker]

Good to know! Thanks! Can you create a small repo to exactly demo your setup? That will make debugging much easier.

[cnelson87]

https://github.com/cnelson87/grunt-browserify-214-test This is my test proj. There's some junk in there you won't need, I just needed some junk to test with. I hope this helps!!

[roncli]

Pull Request has been updated for 1.0.0.

[joeybaker]

@cnelson87 @roncli Hey guys, I finally got a windows VM up and ran the tests. Ya'll were of course, right, the tests didn't pass on windows.

However, the reason was that the tests were assuming a path separator of /, which is not a valid assumption on windows. I've fixed the tests to use path.join where necessary and I now see a full green suite on Windows.

I'm going to close this PR.

[roncli]

Sounds good. I will try some time this week with 1.1.1, and if I still have an issue I'll let you know.

[roncli]

@joeybaker

I wanted to give you an update. I am now using rendr 0.5.1 with remapify 1.3.0. I was able to successfully get this to run in Windows by essentially "throwing out" all of the keys that I am not going to use.

For instance, remapify's expandedAliases includes app/app, app\app, app/app.js, and app\app.js. I just want app/app, so in the remapify:files event, I just filter out anything that has .js or a backslash.

Here's the full grunt setup for browserify that I am now using successfully:

        browserify: {
            combine_rendr_assets: {
                options: {
                    require: Object.keys(pjson.browser),
                    preBundleCB: function(b) {
                        b.plugin(remapify,
                            {
                                cwd: "./app",
                                src: "**/*.js",
                                expose: "app"
                            }
                        );
                        b.on("remapify:files", function(file, expandedAliases) {
                            Object.keys(expandedAliases).forEach(function(key) {
                                if (key.indexOf(".js") === -1 && key.indexOf("\\") === -1) {
                                    console.log(expandedAliases[key], {expose: key});
                                    b.require(expandedAliases[key], {expose: key});
                                }
                            });
                        });
                    }
                },
                files: {
                    "assets/js/mergedAssets.js": ["app/**/*.js"]
                }
            }
        },

There may be a better way for me to do this, but this is a way that is working for me 100% right now.

Thank you again for your work on this project. :)

[joeybaker]

Interesting. Thanks @roncli