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

Resolve builtins with --no-browser-field, fixes #1654 #1826

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions index.js
Expand Up @@ -88,6 +88,10 @@ function Browserify (files, opts) {
self._bresolve = browserField === false
? function (id, opts, cb) {
if (!opts.basedir) opts.basedir = path.dirname(opts.filename)
// Resolve builtin modules.
if (self._mdeps.options.modules[id]) return process.nextTick(function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should use resolve’s isCore functionality, i think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb I don't believe so, as custom built-ins can be fed into Browserify, and those won't be picked up by resolve.isCore. See: https://github.com/browserify/browserify/blob/master/index.js#L551-L553

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If those can override the default ones, then that's a fair point - but then browserify should be using resolve's node-version-specific logic for the defaults, which it doesn't appear to be.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb Yes, it overrides the defaults, in a way. It's just a direct replacement of built-in resolution. Browserify doesn't use the node-version-specific logic, and that's a separate issue from this one that's worth logging. I'm maintaining a fork right now with this change, and I'd like to be able to depend on the officially published version. Is there anything else holding this up or can it move forward?

cb(null, self._mdeps.options.modules[id]);
});
resolve(id, opts, cb)
}
: typeof browserField === 'string'
Expand Down
40 changes: 40 additions & 0 deletions test/browser_field_builtin.js
@@ -0,0 +1,40 @@
var test = require('tap').test;
var fs = require('fs');
var path = require('path');
var vm = require('vm');
var browserify = require('../');
var temp = require('temp');
temp.track();
var tmpdir = temp.mkdirSync({prefix: 'browserify-test'});

test('no browser field builtin', function (t) {
t.plan(2);
var src = fs.readFileSync(path.join(__dirname, '/browser_field_builtin/main.js'));
fs.writeFileSync(path.join(tmpdir, 'main.js'), src);
var b = browserify({
entries: path.join(tmpdir, 'main.js'),
browserField: false
});
b.bundle(function (err, src) {
t.ifError(err);
vm.runInNewContext(src, { console: { log: log } });
function log (msg) { t.deepEqual(msg, { a: 'b' }) }
});
});

test('no browser field excluded builtin', function (t) {
t.plan(2);
var src = fs.readFileSync(path.join(__dirname, '/browser_field_builtin/main.js'));
fs.writeFileSync(path.join(tmpdir, 'main.js'), src);
var b = browserify({
entries: path.join(tmpdir, 'main.js'),
builtins: ['path', 'util'],
browserField: false
});
b.bundle(function (err, src) {
t.ifError(err);
t.throws(function () {
vm.runInNewContext(src, { console: { log: function(){} } });
}, /Cannot find module 'querystring'/)
});
})
1 change: 1 addition & 0 deletions test/browser_field_builtin/main.js
@@ -0,0 +1 @@
console.log(require('querystring').parse('a=b'));