Make sure entry paths are always full paths

[zertosh]

When using the CLI, entry files were always resolved to absolute paths, but when using the API, they weren't. module-deps respects the row.file on its initial rows insofar that it resolved relative paths (based on opts.basedir) but it didn't rewrite it on the row. This led to a weird behavior with watchify where entry files were never being pulled from the cache - unless you used the CLI or passed in an absolute path via the API.

Using basedir to resolve the path is consistent with @substack's view on path resolution ("require vs entry" in #1219 (comment)).

This PR also adds a few tests that use the relative path behavior and extends others to verify that row.file is always the absolute path.

cc: @jmm @terinjokes

[ghost]

Looks great, published in 10.1.1.

[jmm]

@zertosh @substack I think we need to decide exactly how (and where) entry file paths will be resolved and document it more. This PR is a bit different, but reminiscent of the situation with #1033 / #1059 / #1077. This PR pseudo-resolves the paths and module-deps still actually resolves them, so unfortunately this does not ensure that row.file is an absolute pathname that actually exists or the same path that module-deps resolves.

Here's a very simple repro (given an entry.js in cwd):

var
  browserify = require('browserify'),
  row_flow = require('browserify-row-flow');

browserify('./entry')
  .plugin(row_flow().plugin())
  .bundle()
;  

You can see that it exits the record phase (and deps) with file: '/basedir/entry' and is emitted by the file event with file: '/basedir/entry.js' (the value that module-deps actually resolved it to).

Re: #1219 (comment), I almost brought this up over there that the CLI and API resolve entry file paths differently, but I figured it's a divergence that makes sense so I didn't bother. As of v10.1.0 entry files provided to the API were resolved using the module resolution algorithm. So the following were not equivalent:

browserify('./entry');
browserify('entry');

I figure it makes sense for the CLI to resolve entry by appending it to the basedir because that's probably the 80% use case and character count is much more significant than for API calls. With the API, if you want entry paths appended to the basedir instead of put through the module resolution algorithm it's trivial to prepend ./.

This PR changes the resolution behavior of the second case:

browserify('entry');

Previously, if there were a node_modules/entry/index.js, this would've been resolved to it. Now it appends it to basedir.

[zertosh]

Probably the worst behavior is having the API and CLI do different things. Sure, this PR doesn't ensure that the file exists, but it brings consistency.

I think we need to decide exactly how (and where) entry file paths will be resolved and document it more.

The best thing to do is to just delegate to mdeps. This line is what preserves the input row's file. Just remove the if and all rows will have a file prop that is the mdeps resolved path. Want me to look into this?

With the API, if you want entry paths appended to the basedir instead of put through the module resolution algorithm it's trivial to prepend ./

It's still trivial. Just pass in an absolute path :)

[jmm]

Probably the worst behavior is having the API and CLI do different things. Sure, this PR doesn't ensure that the file exists, but it brings consistency.

Maybe. They're pretty different animals though and I'd hate to see compromises made for the CLI pollute the API. The CLI and API would be more consistent if you could do b.require('some_file.js:alias'), but I doubt anyone thinks that's a good idea.

The best thing to do is to just delegate to mdeps.

That's pretty much what I've been thinking and what I did for a similar case in #1077.

This line is what preserves the input row's file. Just remove the if and all rows will have a file prop that is the mdeps resolved path. Want me to look into this?

For me it just circles back to my questions in #1162 -- what is file supposed to represent? Like I said in #1203 (comment) my working theory is that it should "always be a string that represents an absolute pathname, perhaps a sham value for streams or other forms of in-memory rows" (or at least by the time module-deps is done with it it should). So if that really is what it should represent, then yes, making that assignment unconditional would be the thing to do.

It's still trivial. Just pass in an absolute path :)

Well it's even more trivial now since it happens by default. What is now less trivial is using a module name to refer to an entry file. I guess now you'd have to manually invoke node-resolve on it, or if you're sure you want to resolve it from cwd, require.resolve().

[Gillingham]

It looks like this broke being able to name a module and file location using the browser field in package.json What's worse is that with the JSONStream changes trying to downgrade fails due to the missing dependency.

[jmm]

@Gillingham Can you please open an issue with a simple reproduction for the browser field part?

[Gillingham]

@jmm opened #1250