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

roncli · 2014-07-08T22:08:35Z

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.

Currently there is a bug in Windows where all of the expanded aliases will have paths with backslashes in it. This is due to the use of path.join to create the alias, which uses the current platform's path separator. Since Glob is returning paths with forward slash - even on Windows - the solution is to put `pattern.expose + '/'` in front of the file when pattern.expose is present, or just using the file unhindered as the alias if pattern.expose is absent.

Currently when you create aliases, the aliases point to the full file path (ie: `c:\\dev\\workspace\\index.js`). However, this can result in the full file path ending up in the merged file in browserify in some circumstances (rendr has this problem). This simply changes the absolute path to a relative path based on the cwd option.

There are no more references to path, so we no longer need to require it.

joeybaker · 2014-07-10T04:19:26Z

@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 · 2014-07-10T18:37:21Z

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 · 2014-07-10T18:48:41Z

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

roncli · 2014-07-11T00:47:02Z

@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 · 2014-07-14T10:36:42Z

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 · 2014-07-14T16:31:08Z

@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 · 2014-07-14T16:56:28Z

Thanks guys. I'll review and merge soon!

joeybaker · 2014-07-16T03:13:18Z

test/test.js

@@ -79,7 +79,7 @@ describe('remapify', function(){
        , 'path/nested/a.js'
        , 'path/nested/c.js'
      )
-      expandedAliases['path/a.js'].should.equal(path.resolve(__dirname, './fixtures/target/a.js'))
+      expandedAliases['path/a.js'].should.equal('./test/fixtures/target/a.js')


I'm not sure this is a good idea? I agree this causes the tests to pass, but Browserify, unless I'm mistaken, needs absolute paths, Therefore, we should be testing for those, no?

Perhaps what we need here is path.join(__dirname, 'fixtures', 'target', 'a.js') ?

I am using Browserify for building a rendr site on both Mac and Windows, and in my experience it did not need the absolute paths. Prior to the need for Remapify, it was using relative paths then as well.

tleunen · 2014-07-24T03:18:24Z

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 · 2014-07-24T21:12:17Z

@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 · 2014-07-25T14:53:12Z

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

cnelson87 · 2014-07-28T16:51:19Z

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

roncli · 2014-07-28T18:14:53Z

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 · 2014-07-28T20:02:59Z

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

joeybaker · 2014-08-03T18:56:03Z

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

might address #7

joeybaker · 2014-08-09T02:20:03Z

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

cnelson87 · 2014-08-12T00:35:41Z

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 · 2014-08-12T00:38:06Z

@cnelson87 can you show me your remapify config?

cnelson87 · 2014-08-15T01:03:06Z

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 · 2014-08-15T16:44:24Z

@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 · 2014-08-15T20:36:33Z

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 · 2014-08-17T00:05:44Z

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

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

cnelson87 · 2014-08-18T17:22:50Z

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

joeybaker · 2014-08-19T16:27:23Z

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

cnelson87 · 2014-08-19T18:48:24Z

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

joeybaker · 2014-08-19T19:30:16Z

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

cnelson87 · 2014-08-22T17:48:37Z

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!!

Conflicts: lib/remapify.js

roncli · 2014-08-24T05:21:36Z

Pull Request has been updated for 1.0.0.

joeybaker · 2014-09-01T19:16:34Z

@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 · 2014-09-02T14:54:50Z

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 · 2014-12-31T19:05:50Z

@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 · 2015-01-03T20:23:58Z

Interesting. Thanks @roncli

roncli added 3 commits July 8, 2014 11:30

Remove requirement to path

673ff0b

There are no more references to path, so we no longer need to require it.

joeybaker closed this Jul 10, 2014

roncli added 2 commits July 10, 2014 11:21

Bug fixes and test case updates.

45b006c

Tab to space.

14151dd

joeybaker reopened this Jul 10, 2014

roncli changed the title ~~Eliminate use of require("path") to resolve Windows compatibility issues~~ Limit use of require("path") to resolve Windows compatibility issues Jul 11, 2014

joeybaker reviewed Jul 16, 2014
View reviewed changes

This was referenced Jul 17, 2014

Make rendr app mountable under Express 4 rendrjs/rendr#379

Merged

Upgrade dependencies to the latest modules rendrjs/rendr-examples#6

Closed

joeybaker added a commit that referenced this pull request Aug 3, 2014

Always alias to absolute paths

eaf8945

might address #7

roncli added 4 commits August 23, 2014 23:53

Merge commit '86f32452d05d5508b9d4418b856479078bbbee49'

28d9631

Merge branch 'upstream/master'

9b0decc

Conflicts: lib/remapify.js

This linter though.

4c184f1

Thought I deleted the whole line, guess not.

e7a4846

joeybaker closed this Sep 1, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

roncli commented Jul 8, 2014

joeybaker commented Jul 10, 2014

roncli commented Jul 10, 2014

joeybaker commented Jul 10, 2014

roncli commented Jul 11, 2014

danielgutenberg commented Jul 14, 2014

roncli commented Jul 14, 2014

joeybaker commented Jul 14, 2014

joeybaker Jul 16, 2014

roncli Jul 16, 2014

tleunen commented Jul 24, 2014

roncli commented Jul 24, 2014

roncli commented Jul 25, 2014

cnelson87 commented Jul 28, 2014

roncli commented Jul 28, 2014

joeybaker commented Jul 28, 2014

joeybaker commented Aug 3, 2014

joeybaker commented Aug 9, 2014

cnelson87 commented Aug 12, 2014

joeybaker commented Aug 12, 2014

cnelson87 commented Aug 15, 2014

joeybaker commented Aug 15, 2014

cnelson87 commented Aug 15, 2014

joeybaker commented Aug 17, 2014

cnelson87 commented Aug 18, 2014

joeybaker commented Aug 19, 2014

cnelson87 commented Aug 19, 2014

joeybaker commented Aug 19, 2014

cnelson87 commented Aug 22, 2014

roncli commented Aug 24, 2014

joeybaker commented Sep 1, 2014

roncli commented Sep 2, 2014

roncli commented Dec 31, 2014

joeybaker commented Jan 3, 2015

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

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

Conversation

roncli commented Jul 8, 2014

joeybaker commented Jul 10, 2014

roncli commented Jul 10, 2014

joeybaker commented Jul 10, 2014

roncli commented Jul 11, 2014

danielgutenberg commented Jul 14, 2014

roncli commented Jul 14, 2014

joeybaker commented Jul 14, 2014

joeybaker Jul 16, 2014

Choose a reason for hiding this comment

roncli Jul 16, 2014

Choose a reason for hiding this comment

tleunen commented Jul 24, 2014

roncli commented Jul 24, 2014

roncli commented Jul 25, 2014

cnelson87 commented Jul 28, 2014

roncli commented Jul 28, 2014

joeybaker commented Jul 28, 2014

joeybaker commented Aug 3, 2014

joeybaker commented Aug 9, 2014

cnelson87 commented Aug 12, 2014

joeybaker commented Aug 12, 2014

cnelson87 commented Aug 15, 2014

joeybaker commented Aug 15, 2014

cnelson87 commented Aug 15, 2014

joeybaker commented Aug 17, 2014

cnelson87 commented Aug 18, 2014

joeybaker commented Aug 19, 2014

cnelson87 commented Aug 19, 2014

joeybaker commented Aug 19, 2014

cnelson87 commented Aug 22, 2014

roncli commented Aug 24, 2014

joeybaker commented Sep 1, 2014

roncli commented Sep 2, 2014

roncli commented Dec 31, 2014

joeybaker commented Jan 3, 2015