Dynamically calculate dirname and filename when --node is passed #1725

jviotti · 2017-05-17T20:24:15Z

When --no-commondir is set (as a consequence of --node for example),
it will cause the final bundle to hardcode absolute values of the
machine that generated the bundle in order to resolve __dirname and
__filename. For example, consider a foo.js file containing:

console.log(__dirname);
console.log(__filename);

Calling browserify --node on it results in:

(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
(function (__filename,__dirname){
console.log(__dirname);
console.log(__filename);

}).call(this,"/Users/jviotti/Projects/playground/node-browserify/foo.js","/Users/jviotti/Projects/playground/node-browserify")
},{}]},{},[1]);

Notice the absolute paths at the end of the bundle. This means that
Browserify node users can't generate a bundle in one machine, and expect
it to run without issues on another machine.

As a solution, we can use the final bundle's __dirname to dynamically
resolve every file's __dirname and __filename. This change only
takes place when --node is set, and keeps Browserify backwards
compatible.

For example, the above output might contain the following line:

}).call(this,require("path").join(__dirname,"foo.js"),require("path").join(__dirname,"."))

Fixes: #1723
See: balena-io/etcher#1429
See: balena-io/etcher#1409
Signed-off-by: Juan Cruz Viotti jviotti@openmailbox.org

We encountered a Browserify bug when releasing the CLI, where absolute paths would be hard-coded in the final bundle. Since we were in the middle of a release process, we added a quick and dirty search-and-replace workaround on `concatenate-javascript.sh`. After the release, we submitted a PR to Browserify which fixes the issue. This commit makes use of my personal fork to be able to use such fix while it gets merged to the main project. See: browserify/browserify#1725 See: #1409 Fixes: #1429 Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>

lurch · 2017-05-18T00:23:48Z

index.js

+              return 'require("path").join(__dirname,"' + dir.split(path.sep).join('","') + '")';
+            },
+            __filename: function(file) {
+              var filename = path.relative(basedir, file);


Possibly-stupid first question:
How come __dirname has the parameters (file, basedir), but __filename only has the parameter file? In that case, where does the basedir variable in this line come from?

Oops, fixed!

So how come this wasn't being picked up by the tests? Does it imply that it'd be worth testing __dirname and __filename separately, rather than testing them together?

Looks like there is another basedir variable defined above (https://github.com/substack/node-browserify/blob/master/index.js#L428):

var basedir = defined(opts.basedir, process.cwd());

And that happens to have the right one as well (its basically what gets passed to the __filename function).

I'll see if I can somehow break it and test it.

I give up. The variable declared above is always valid, and I can't find a way to break it. I guess a linter should be the correct way to detect these kinds of subtle issues.

Yeah, I guess only a linter is able to automatically spot 'conflicting' variable names and suggest that one of them gets renamed, but that's obviously well outside the scope of this PR ;-)

lurch · 2017-05-18T00:26:54Z

test/bare.js

@@ -68,3 +75,72 @@ test('bare inserts __filename,__dirname but not process,global,Buffer', function
        t.equal(code, 0);
    });
 });
+
+test('bare inserts dynamic __filename,__dirname', function (t) {


Possibly-stupid second question:
The test description mentions both __filename and __dirname, but AFAICT this is only testing __dirname?

These tests execute the following file: https://github.com/substack/node-browserify/pull/1725/files#diff-df4230a729c19a353d1405d5459df60d, which prints both __dirname, and __filename (and we assert the result of both)

We encountered a Browserify bug when releasing the CLI, where absolute paths would be hard-coded in the final bundle. Since we were in the middle of a release process, we added a quick and dirty search-and-replace workaround on `concatenate-javascript.sh`. After the release, we submitted a PR to Browserify which fixes the issue. This commit makes use of my personal fork to be able to use such fix while it gets merged to the main project. See: browserify/browserify#1725 See: #1409 Fixes: #1429 Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>

inDream · 2017-08-04T07:46:38Z

@substack @feross When will this fix be merged?

AKK9 · 2017-12-12T11:31:12Z

Is it possible to have this fixed?

goto-bus-stop · 2017-12-16T13:01:39Z

index.js

+          opts.insertGlobalVars = xtend({
+            __dirname: function(file, basedir) {
+              var dir = path.dirname(path.relative(basedir, file));
+              return 'require("path").join(__dirname,"' + dir.split(path.sep).join('","') + '")';


paths can contain " on some filesystems, could you use JSON.stringify here instead of manually wrapping the path in quotes?

Sounds good, done!

goto-bus-stop · 2017-12-16T13:03:23Z

index.js

@@ -551,6 +552,19 @@ Browserify.prototype._createDeps = function (opts) {
                return through();
            }
        }
+
+        if (opts.commondir === false && opts.builtins === false) {
+          opts.insertGlobalVars = xtend({


could this be done in cmd/args.js, in the same place as the other node: option stuff?

Not sure I understand this comment. Do you mean setting commondir: false and builtins: false if node: true, and then just check for node: true here?

oh sorry nevermind, i misunderstood what this check is doing--i thought you were using this to essentially check if --node was passed, but opts.commondir actually is the one causing the issue.

When `--no-commondir` is set (as a consequence of `--node` for example), it will cause the final bundle to hardcode absolute values of the machine that generated the bundle in order to resolve `__dirname` and `__filename`. For example, consider a `foo.js` file containing: ```js console.log(__dirname); console.log(__filename); ``` Calling `browserify --node` on it results in: ```js (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){ (function (__filename,__dirname){ console.log(__dirname); console.log(__filename); }).call(this,"/Users/jviotti/Projects/playground/node-browserify/foo.js","/Users/jviotti/Projects/playground/node-browserify") },{}]},{},[1]); ``` Notice the absolute paths at the end of the bundle. This means that Browserify node users can't generate a bundle in one machine, and expect it to run without issues on another machine. As a solution, we can use the final bundle's `__dirname` to dynamically resolve every file's `__dirname` and `__filename`. This change only takes place when `--node` is set, and keeps Browserify backwards compatible. For example, the above output might contain the following line: ``` }).call(this,require("path").join(__dirname,"foo.js"),require("path").join(__dirname,".")) ``` Fixes: #1723 See: balena-io/etcher#1429 See: balena-io/etcher#1409 Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org> Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

goto-bus-stop

lgtm, thanks for your patience 🙏 do you think we need a major version bump for this?

jviotti · 2017-12-26T13:26:29Z

Awesome, thanks a lot and sorry for the delay addressing the comments!

jviotti · 2017-12-26T13:27:50Z

@goto-bus-stop

do you think we need a major version bump for this?

Since all the previous tests are passing, this is a backwards compatible change, which I guess should be minor version bump.

jviotti mentioned this pull request May 17, 2017

fix(CLI): get rid of Browserify absolute path workaround balena-io/etcher#1449

Merged

lurch reviewed May 18, 2017

View reviewed changes

jviotti mentioned this pull request May 30, 2017

pkg stores absolute paths from the machine the executable was generated from vercel/pkg#116

Closed

goto-bus-stop self-requested a review December 12, 2017 15:30

goto-bus-stop reviewed Dec 16, 2017

View reviewed changes

goto-bus-stop approved these changes Dec 26, 2017

View reviewed changes

goto-bus-stop merged commit 2a574e4 into browserify:master Feb 7, 2018

jviotti deleted the dynamic-dirname-filename branch February 7, 2018 17:08

dependabot-preview bot mentioned this pull request Mar 14, 2018

Bump browserify from 15.2.0 to 16.1.1 tobybellwood/govstrap#3

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically calculate dirname and filename when --node is passed #1725

Dynamically calculate dirname and filename when --node is passed #1725

jviotti commented May 17, 2017 •

edited

lurch May 18, 2017

jviotti May 18, 2017

lurch May 18, 2017

jviotti May 20, 2017

jviotti May 20, 2017

lurch May 20, 2017

lurch May 18, 2017

jviotti May 18, 2017

inDream commented Aug 4, 2017

AKK9 commented Dec 12, 2017

goto-bus-stop Dec 16, 2017

jviotti Dec 25, 2017

goto-bus-stop Dec 16, 2017

jviotti Dec 25, 2017

goto-bus-stop Dec 26, 2017

goto-bus-stop left a comment

jviotti commented Dec 26, 2017

jviotti commented Dec 26, 2017

Dynamically calculate __dirname and __filename when --node is passed #1725

Dynamically calculate __dirname and __filename when --node is passed #1725

Conversation

jviotti commented May 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inDream commented Aug 4, 2017

AKK9 commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goto-bus-stop left a comment

Choose a reason for hiding this comment

jviotti commented Dec 26, 2017

jviotti commented Dec 26, 2017

Dynamically calculate dirname and filename when --node is passed #1725

Dynamically calculate dirname and filename when --node is passed #1725

jviotti commented May 17, 2017 •

edited