I was looking at the test, I thought maybe something like this would be easier to understand, but I leave it up to you - feel free to ignore this comment:

// don't use unicode/main.js - this is just for demo purposes
test('b.require transform prop file path', function (t) {
    t.plan(2);
    var b = browserify();
    b.require([{
        file: __dirname + '/unicode/main.js',
        transform: through(function() {
            t.fail('this should not be called');
        })
    }]);
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });
});

test('b.require transform prop file path', function (t) {
    t.plan(2);
    var b = browserify();
    b.require(__dirname + '/unicode/main.js', {
        transform: through(function() {
            t.fail('this should not be called');
        })
    });
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });
});

test('b.require transform prop file stream', function (t) {
    t.plan(2);
    var s = through();
    var b = browserify();
    b.require(s, {
        transform: through(function() {
            t.fail('this should not be called');
        })
    });
    s.write('module.exports = {};');
    s.end();
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });
});

@zertosh

You're mutating the object that was passed into require. This should be done after the xtend below.

Ok. The only reason I did that was to "head it off at the pass" and not have to do it separately for stream / non-stream, but point taken. I wanted to do some general refactoring on this method at some point to consolidate the stream / non-stream logic better in general, unify on usage of row|rec for both paths, etc. So I fixed this and then did another commit with more general refactoring that consolidates this logic again but doesn't mutate the args. This also provides a single location to whitelist / blacklist any other props.

I don't think the general refactoring should break anything, but if you have the chance to look it over that'd be great. I'm a little bit concerned about the test coverage. I was trying to make it so the order value only gets incremented for streams when it'll be used (just to keep them sequential), so I originally made that conditional on opts.entry, but then I realized that it's used for the sham file name even if the stream isn't an entry file. There apparently isn't a test to catch that.

I was looking at the test, I thought maybe something like this would be easier to understand

That's not a bad idea. I could kind of go either way, but doing it the way you suggest gives it a narrower focus, which probably isn't bad in this case. I kept the general structure I had with a parameterized function to avoid duplication (not sure if you were even suggesting changing that) but simplified it in the way you suggested: made the entry file content irrelevant and the transform a passthrough and just t.fail() if the transform function is called, and t.pass() if it makes it to the bundle() callback without error. I think t.plan(1) => maybe t.fail() => t.pass() is fine. I would've done t.passing() && t.pass() just to make sure, since the documentation doesn't explain it well enough, but it's not available in this version.

Here's an updated branch:

Part that corresponds to original PR branch:
master...jmm:bbb1ab8

And here's the general refactoring on top of that:
jmm/node-browserify@bbb1ab8...b-require-transform2

The last commit, the general refactoring, could be a separate PR if necessary.

I don't think the general refactoring should break anything, but if you have the chance to look it over that'd be great. I'm a little bit concerned about the test coverage.

Both delete row.transform; look good, but I had a hard time following the refactoring. Why not simply call b.require again when the stream has been concatenated? All you need to add to the opts is filename and order, then everything else will get added on the 2nd pass of b.require. But we shouldn't touch this without adding a few more tests.

I kept the general structure I had with a parameterized function to avoid duplication (not sure if you were even suggesting changing that)

Yeah, I'm sorry - I don't mean to walk all over your PR :( The substack philosophy seems to emphasize duplication. Each spec is pretty self-contained with only minor exceptions. Also, each test seems to have their own fixtures, not a lot of sharing there either.

IIRC, when using tap, until the plan count is met, you'll just sit around waiting for a timeout. So by using t.ifError(err); t.ok(src);, you "catch" errors instantly and with a message, instead of waiting for a timeout. If the b.bundle() errored for any reason other than the transform we're testing for getting called, you'll sit around for 10 seconds waiting for the test to timeout.

This is just a suggestion, please ignore it if you disagree: I'd make a test/require_transform/main.js with a dummy line, then write out the tests like this:

test('b.require() file path with file.transform prop', function (t) {
    t.plan(2);
    var b = browserify();
    var file = __dirname + '/require_transform/main.js';
    b.require([{
        file: file,
        transform: through(function() {
            t.fail('this should not be called');
        })
    }]);
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });
});

test('b.require() file path with opts.transform prop', function (t) {
    t.plan(2);
    var b = browserify();
    var file = __dirname + '/require_transform/main.js';
    b.require(file, {
        transform: through(function() {
            t.fail('this should not be called');
        })
    });
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });
});

test('b.require() file stream with file.transform prop', function (t) {
    t.plan(2);
    var b = browserify();
    var file = fs.createReadStream(__dirname + '/require_transform/main.js');
    b.require([{
        file: file,
        transform: through(function() {
            t.fail('this should not be called');
        })
    });
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });  
})

test('b.require() file stream with opts.transform prop', function (t) {
    t.plan(2);
    var b = browserify();
    var file = fs.createReadStream(__dirname + '/require_transform/main.js');
    b.require(file, {
        transform: through(function() {
            t.fail('this should not be called');
        })
    });
    b.bundle(function(err, src) {
        t.ifError(err);
        t.ok(src);
    });
});

but I had a hard time following the refactoring.

In what way?

Why not simply call b.require again when the stream has been concatenated?

Only because I didn't think of it, and it's a more dramatic refactoring (not that that's a bad thing). Maybe that is a better solution. If so, I'm all for it.

All you need to add to the opts is filename and order, then everything else will get added on the 2nd pass of b.require.

I'll have to check it out and make sure everything would come out right. The logic in that method is very convoluted, almost entirely lacking comments, and has questionable test coverage. Aside from that, you'd have to do something (maybe a private method and a public method) to prevent users from populating order, right?

Yeah, I'm sorry - I don't mean to walk all over your PR

I'm always open to constructive criticism and improvement.

:( The substack philosophy seems to emphasize duplication. Each spec is pretty self-contained with only minor exceptions.

My philosophy is generally to very much emphasize non-duplication. These tests are very closely related variations on a theme, differing only by a couple of parameters.

Also, each test seems to have their own fixtures, not a lot of sharing there either.

Yeah, I know that was a bit hokey to use that test/tr/package.json file, but I literally just needed a file that could be resolved and bundled. That's its only significance to the test. But since it concerns you I'll remedy it.

IIRC, when using tap, until the plan count is met, you'll just sit around waiting for a timeout. So by using t.ifError(err); t.ok(src);, you "catch" errors instantly and with a message, instead of waiting for a timeout. If the b.bundle() errored for any reason other than the transform we're testing for getting called, you'll sit around for 10 seconds waiting for the test to timeout.

I was under the impression that the way I authored it would avoid waiting for timeout. What does the tap documentation for ifError() mean where it says:

Note: if an error is encountered unexpectedly, it's often better to simply throw it. The Test object will handle this as a failure.

If there's err in this case it's unexpected, because something other than what the test was testing for went wrong. I tried making the entry file invalid and it fails immediately:

entry.end(Buffer('";'));

test/require_transform.js:74
          if (err) throw err;
                         ^
SyntaxError: Unterminated string constant

This is just a suggestion, please ignore it if you disagree: I'd make a test/require_transform/main.js with a dummy line, then write out the tests like this:

I'll make an entry file specifically for these tests. I value your feedback, but I have to respectfully disagree that duplicating the bulk of the test code n times is better. In my opinion it's much worse, so I'm not going to do that. I understand that you disagree with that, so don't worry about merging these changes, I'll do it.

@zertosh

Why not simply call b.require again when the stream has been concatenated? All you need to add to the opts is filename and order, then everything else will get added on the 2nd pass of b.require. But we shouldn't touch this without adding a few more tests.

This is approximately what I see that looking like (based on master, tests passing):
master...jmm:b-require-refactor-temp

I think it probably is better.

it's a more dramatic refactoring (not that that's a bad thing)

Heh, I think it's actually the simplest solution https://gist.github.com/zertosh/e66a3e96afc45228f02b

The logic in that method is very convoluted, almost entirely lacking comments, and has questionable test coverage. Aside from that, you'd have to do something (maybe a private method and a public method) to prevent users from populating order, right?

"Questionable test coverage" lol - so true. All of the expose logic above it doesn't make sense for streams:

// never true
if (file === expose && /^[\.]/.test(expose)) {
    expose = '/' + path.relative(basedir, expose);
}
// if this is true.....
if (expose === undefined && this._options.exposeAll) {
    expose = true;
}
// then this will throw because `path.relative` only
// takes string arguments - file -> stream
if (expose === true) {
    expose = '/' + path.relative(basedir, file);
}

I not sure about making public/private methods. If you mess with order then you deserve what you've got coming to you hehe. Not a huge concern though, I think the streams logic here is the for the CLI - it passes process.stdin as the file stream. BUT once the row props are figured out, something like order should be renamed to _order.

My philosophy is generally to very much emphasize non-duplication. These tests are very closely related variations on a theme, differing only by a couple of parameters.

Don't get me wrong, I'm not in favor of code duplication - except in tests :P Tests have to be the easiest possible to understand, with very little surface area for errors in themselves. Introducing abstractions or syntax works against that. But I won't block you over testing philosophy.

I was under the impression that the way I authored it would avoid waiting for timeout

Yup. You're right. I just verified it 👍

Heh, I think it's actually the simplest solution https://gist.github.com/zertosh/e66a3e96afc45228f02b

The end result probably is. But it's a more dramatic refactoring because there's more to consider. For example, in your gist, does elimination of the self._expose[id] assignment break anything? I ask because I don't know off the top of my head and it's another thing I'd have to check into before I eliminated it in a refactor.

"Questionable test coverage" lol - so true. All of the expose logic above it doesn't make sense for streams:

Yeah, I also moved the stream handling logic above that in my refactor that based on your idea of re-calling b.require().

I not sure about making public/private methods. If you mess with order then you deserve what you've got coming to you hehe.

I guess at this point I feel like we should do one thing or the other: either filter the props coming from the user or just document the methods well and say: "If you pass undocumented props then un(intended|wanted|documented) behavior may occur, and that behavior is subject to change. Don't create issues involving undocumented use of props."

I think the streams logic here is the for the CLI - it passes process.stdin as the file stream.

Well, not all of it, right?:

b.require(file, opts)

file can also be a stream, but you should also use opts.basedir so that relative requires will be resolvable.

BUT once the row props are figured out, something like order should be renamed to _order.

Just to provide a hint?

Anyway, I'm going to hold off on making any changes related to these transform props until the dust settles on refactoring the method in general.

does elimination of the self._expose[id] assignment break anything?

You made it so that module-deps does that for you :)

"If you pass undocumented props then un(intended|wanted|documented) behavior may occur, and that behavior is subject to change. Don't create issues involving undocumented use of props."

I reallllyyyy like this policy - more so than filtering.

I think the streams logic here is the for the CLI - it passes process.stdin as the file stream.

Well, not all of it, right?:

I guess what I meant was: "I think the only user of this API is the CLI". I don't see 99.9% of users every touching this functionality.

Just to provide a hint?

Yeah, an idiomatic hint. It goes very well with the above policy statement.

Anyway, I'm going to hold off on making any changes related to these transform props until the dust settles on refactoring the method in general.

I think removing the props now will help with the refactor, since if anything breaks, it'll help guide the refactor. Just a thought.

You made it so that module-deps does that for you :)

Yeah, I just didn't know if that gets invoked for a row with a source property. I just took a look and I guess it does.

I reallllyyyy like this policy - more so than filtering.

We could do that. Of course, people are still going to publish blog posts and stuff instructing people to use undocumented props and they're still going to post issues about it here.

I guess what I meant was: "I think the only user of this API is the CLI". I don't see 99.9% of users every touching this functionality.

Perhaps, but nonetheless it's a documented part of the public API.

Yeah, an idiomatic hint. It goes very well with the above policy statement.

Ok, we should keep that in mind for future consideration.

I think removing the props now will help with the refactor, since if anything breaks, it'll help guide the refactor. Just a thought.

If we're not going to filter props and just have a hands-off policy instead then we don't need any of these commits (other than the general refactoring ones).

If we're not going to filter props and just have a hands-off policy instead then we don't need any of these commits (other than the general refactoring ones).

I guess so. Do you want the policy to the read me and/or the "submitting issues" wiki page?

@zertosh

Do you want the policy to the read me and/or the "submitting issues" wiki page?

Are you asking where I think it should go, or whether I want to do it? I think it should be in the readme and I think adding it to the "submitting issues" wiki is a great idea. I'll add it in either or both places if you want.

Are you asking where I think it should go, or whether I want to do it?

Both? Either? Go for it in both! 😄