Fix multi-entry with cross-require

[zertosh]

Fixes #1266. I broke it in #1248. What was happening was that I was setting row.file too late, and row.id was not getting the full path. The example in #1266 would've worked via the CLI before #1248, but not via the API. This PR fixes that.

@jmm I really want to merge the refactor of b.require from https://gist.github.com/zertosh/e66a3e96afc45228f02b (taking these changes into account). What tests do you think are missing before we can do it w/o fear of major breakage?

[jmm]

Some that come to mind (don't know which already exist):

• Correct order value for streams irrespective of when they finish. What's a good way to simulate a "slow" stream? Can you just do this?:

  var stream = through2();
  stream.end("whatever");
  b.require(stream);
  stream.pause();
  b.require('./whatever');
  stream.resume();

• Correct sham file name (incorporating order) for streams.

• Successful exposure of stream (as in b.require(stream, {expose: 'whatever'}).

[jmm]

Re: the "slow" stream, actually I guess you could just do:

var stream = through2();
stream.write("whatever");
b.require(stream);
b.require('./whatever');
stream.end();

[zertosh]

Thanks! Those test ideas make sense. I'll write those up before continuing on a refactor, but we should merge this in as a stopgap though.

[jmm]

@zertosh

but we should merge this in as a stopgap though.

FWIW I still have the concerns I voiced in #1248 (comment), #1248 (comment).

[zertosh]

@jmm So to summarize your concerns:

• row.file needs to be something consistent.
• path resolution when calling b.require needs to be clear (whether it's fs-like or require-like, doesn't matter - just clear).

We just need decisions, right? Because it's not a technical problem - it's easy to code up. How do we decide? Is it our place to decide? And are you ok if I merge this to fix the immediate problem?

[jmm]

@zertosh

• row.file needs to have well-defined, and ideally consistent, semantics and type. Same for row.id.
• path resolution semantics for "root" files (the ones that the dependency graph starts from -- files, opts.entries, b.add(), opts.require, b.require()) need to be clear, including documentation and tests. Even Make sure entry paths are always full paths #1248 illustrates how easy it is to get tripped up currently, as it refers to resolving values to absolute paths, which wasn't actually the case before or after (and has important implications for consistency elsewhere).
• In my opinion decisions about the API should be made with little regard for the CLI. API consistency with the CLI is not an imperative. A well-designed API should be the imperative. Ultimately the CLI is just a consumer of the API that needs to special-case and pre-process its input to massage it into the appropriate format for passing to API calls. Sometimes there's enough overlap between what the CLI and API are doing that the code can just live in the API implementation and doesn't need to be handled specially by the CLI and that's a bonus.

We just need decisions, right?

Right.

How do we decide? Is it our place to decide?

I think it's fruitful for collaborators or any interested parties to discuss it, but if it's not clear cut what should happen I suppose @substack would need to make the call.

And are you ok if I merge this to fix the immediate problem?

Ultimately it's your decision as you don't need my approval. Until these issues get legitimately dealt with I think it would make more sense to revert to the previous situation, where the CLI pseudo-resolved the paths. It's important to keep in mind that #1248 changed the resolution behavior of the API. Maybe the previous resolution behavior was an accident, maybe not, but I think we should try to minimize churn until those questions are deliberately decided. Right now we're chasing our tails with these kinds of changes. It wouldn't surprise me if these changes, while fixing one problem, cause other unexpected problems, even beyond the already noted change to resolution behavior.