Fix external relative paths on Windows by Shingyx · Pull Request #1704 · browserify/browserify

Shingyx · 2017-03-20T03:34:22Z

Replace backslashes with forward slashes when adding relative paths for external references.

Shingyx · 2017-03-20T03:43:12Z

Fixes the following scenario:

Set up bundle A which includes a src file './lib/foo.js'.
Set up bundle B which includes a src file './test/bar.js' and marks './lib/foo.js' as an external dependency.
If './test/bar.js' has the statement require('../lib/foo'), it'll throw the error "Cannot find module '/lib\foo'".

Shingyx · 2017-03-20T04:01:01Z

Build is failing for the same reason that the master branch is failing https://travis-ci.org/substack/node-browserify/builds/207173302

Shingyx · 2017-03-21T07:12:05Z

grunt-test.zip
Attached is a small test project which reproduces this problem.

Shingyx · 2017-03-21T09:42:27Z

...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 ./lib/foo.js in the second bundle, so my change probably breaks the functionality of the external flag and I was misusing Browserify the whole time. Will close if this is the case.

Shingyx · 2017-03-21T21:05:31Z

Can confirm. All my change seemed to do was invalidate the external option, making the second bundle just include the "external" files. However, I think my scenario suggests that there could be an issue somewhere in Browserify's external option, given the error Cannot find module '/lib\foo' was thrown, but my change does nothing to address this issue (if there is one at all).

dwelle · 2017-03-24T16:38:30Z

I can confirm the backslashes on windows are causing problems. Why did you conclude your proposed change is incorrect?

Related #1584, which was not resolved either.
Also #1613, #1511 and numerous other issues

Shingyx · 2017-03-28T06:33:09Z

Firstly, ignore my previous grunt-test.zip. It was more complicated than necessary.

grunt-test.zip
This is a much simpler project which demonstrates the problem. For context, I want to bundle a bunch of production code inside lib-bundle.js and the test code inside test-bundle.js. For those tests, I want the dependencies to come from lib-bundle.js, AKA exactly what will be used in production. I use index.html to run those tests (usually would be using Mocha or something).

dist-before.zip
This zip contains those two bundles before my changes. This is everything I wanted, except the last line of test-bundle.js has {"../lib/library":"/lib\\library.js"}]}, saying 'to import "../lib/library", look up "/lib\library.js"'. As a result, opening index.html will give "Uncaught Error: Cannot find module '/lib\library.js'". Simply replacing the \\ on this line with / gives the expected behaviour since "/lib/library.js" is declared in lib-bundle.js, which is what I was aiming to do.

dist-after.zip
This zip contains the bundles after my changes. lib-bundle.js is unchanged, but test-bundle.js just decided to import the files I was trying to mark as external in itself, suggesting that all my changes did was invalidate the external option.

So while I think there is a problem in this use case, my changes didn't fix it.

dwelle · 2017-03-28T11:03:13Z

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

Shingyx · 2017-04-01T07:10:40Z

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

Shingyx · 2017-04-22T05:18:45Z

Is there anything else I can do to help get this merged in?

kaushikvijay · 2017-10-06T09:46:32Z

I am also facing same issue. Will this get merged?

Shingyx · 2017-10-10T04:33:56Z

@kauvj I haven't heard anything, but I merged in the current master branch to get this to build.

mafintosh · 2017-10-10T04:41:13Z

index.js

    var expose = opts.expose;
    if (file === expose && /^[\.]/.test(expose)) {
        expose = '/' + relativePath(basedir, expose);
-        expose = expose.replace(/\\/g, '/');


Couldn't you use your helper function from below here instead?

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.

mafintosh · 2017-10-10T04:41:29Z

index.js

    }
    if (expose === true) {
        expose = '/' + relativePath(basedir, file);
-        expose = expose.replace(/\\/g, '/');


Same as above

goto-bus-stop

Thanks! this fixes some tests on windows too 🎉

goto-bus-stop · 2018-03-23T08:52:41Z

Will merge this in #1819. Thanks!

Shingyx · 2018-03-23T08:54:43Z

Awesome. Thanks!

Fix external relative paths on Windows

eceac40

Replace backslashes with forward slashes when adding relative paths for external references.

Shingyx closed this Mar 21, 2017

Shingyx added 2 commits April 1, 2017 19:53

Replace all \ with / in relative paths for OS-independent behavior

775e5b4

Add simple .gitignore

1f3cb40

Ignore node_modules directory

Shingyx reopened this Apr 1, 2017

Shingyx added 2 commits April 1, 2017 20:24

Formatting

d292dce

Make relativePath function a bit clearer

51c7e8e

Use the same arguments as the function exported by cached-path-relative, rather than calling .apply

Merge https://github.com/browserify/browserify

4a558e3

mafintosh reviewed Oct 10, 2017

View reviewed changes

Shingyx added 2 commits February 20, 2018 22:01

Merge branch 'master' into master

64cb665

Merge branch 'master' into master

a689437

goto-bus-stop approved these changes Mar 22, 2018

View reviewed changes

goto-bus-stop mentioned this pull request Mar 22, 2018

Passing tests on Windows and Node 10 #1819

Merged

goto-bus-stop closed this Mar 23, 2018

snyk-bot mentioned this pull request Dec 8, 2019

[Snyk] Upgrade browserify from 11.2.0 to 16.5.0 drakeg/choosealicense.com#1

Merged

renovate bot mentioned this pull request May 29, 2021

fix(deps): update dependency browserify to v17 - abandoned ff6347/AEMap#21

Open

1 task

renovate bot mentioned this pull request Jun 14, 2021

Update dependency browserify to v17 Automattic/vip-go-mu-plugins#2212

Merged

1 task

mend-for-github-com bot mentioned this pull request Jul 5, 2021

Update dependency browserify to v17 yoswein/vexflow#499

Open

1 task

renovate bot mentioned this pull request Jul 8, 2021

Update dependency browserify to v17 MohamedElashri/Zotero-Docker#113

Open

1 task

Conversation

Shingyx commented Mar 20, 2017

Uh oh!

Shingyx commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shingyx commented Mar 20, 2017

Uh oh!

Shingyx commented Mar 21, 2017

Uh oh!

Shingyx commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shingyx commented Mar 21, 2017

Uh oh!

dwelle commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shingyx commented Mar 28, 2017

Uh oh!

dwelle commented Mar 28, 2017

Uh oh!

Shingyx commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shingyx commented Apr 22, 2017

Uh oh!

kaushikvijay commented Oct 6, 2017

Uh oh!

Shingyx commented Oct 10, 2017

Uh oh!

mafintosh Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

Shingyx Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

mafintosh Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

goto-bus-stop left a comment

Choose a reason for hiding this comment

Uh oh!

goto-bus-stop commented Mar 23, 2018

Uh oh!

Shingyx commented Mar 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Shingyx commented Mar 20, 2017 •

edited

Loading

Shingyx commented Mar 21, 2017 •

edited

Loading

dwelle commented Mar 24, 2017 •

edited

Loading

Shingyx commented Apr 1, 2017 •

edited

Loading