"fullPaths" optional again

[zertosh]

This diff makes fullPaths optional again. Instead of waiting for browserify to emit dep via emit-deps, we get the rows immediately after deps - but not the entire row, just the parts that are relevant for caching. Since module-deps only cares about id, source, deps and file from a cache perspective.

This also fixes an issue whereby, after each re-build, every .json file would get an extra module.exports= prepended (see #152). This happened because cached files where being re-fed into the pipeline that had already been through the json step.

It's also unnecessary to fs.stat each file. It is a given that a row emitted by module-deps has a file property which corresponds to a file and that it exists.

Disclaimer: I don't know if there was an obscure underlying reason that I might've missed for why watchify observed dep instead of tapping into the pipeline.

cc: @substack @feross @jmm

[ghost]

I think listening for 'dep' was mostly because watchify predates the pipeline and 'dep' was an older interface. The patch looks great, taking it for a spin now.

[ghost]

Passes all the tests, seems to work! Released as 2.5.0 and I added you as a contributor on github and npm: https://gist.github.com/substack/e205f5389890a1425233

[zertosh]

Thanks @substack! 😄

[jmm]

@zertosh @substack
I have to take a closer look at this PR later (just for my own education), but a few things I think are noteworthy:

1. This is the kind of thing I was talking about in Pipeline core sub-phases should be addressable by something "permanent" browserify#1158. For example, I have a plugin (pathmodify) that is designed to push a stream that immediately follows module-deps. Like I said, I need to take a closer look at this PR to see if / how it'll affect my plugin (perhaps not at all).

2. As a general rule I think it would be a good idea if browserify and any plugins (I mean anything public, like on npm) / wrappers (like watchify) / whatever that add streams to the pipeline label them and document that as part of their API. I'm going to do this with my plugin. Probably a good convention would be something like {{npm-pkg-name}}:whatever.

3. In response to Is there a way to specify a file extension to be treated like json browserify#1156 I suggested calling b.pipeline.get('deps').push() to push a stream to alter the row.file property to lie about the filename so it would be treated as JSON, although I admit I'd need to look into that more to appreciate all of the ramifications (even before this change). (I also started looking into modifying my plugin to allow changing properties like row.file following module-deps.)

4. Not sure yet if it matters here, but re: this:

It's also unnecessary to fs.stat each file. It is a given that a row emitted by module-deps has a file property which corresponds to a file and that it exists.

module-deps sometimes emits file properties that are absolute pathnames, sometimes not. See browserify/browserify/issues/1162

[zertosh]

@jmm wow pathmodify is pretty clever - I don't think this will affect it because once path A has been resolved to B, trying to resolve B will still yield B. However, worst case scenario, you have to add the plugins in a certain order - not unlike transforms.

whatever that add streams to the pipeline label them

👍 - but is possible with labeled-stream-splicer to add new labels? Or would you have to re-create the whole pipeline?

I'd need to look into that more to appreciate all of the ramifications

I think step 2 should've have been pushed after the json pipeline step. I think that'll cause double "module.exports=" to be prepended.

module-deps sometimes emits file properties that are absolute pathnames, sometimes not.

Interesting. It shouldn't matter from a watchify perspective because the value of row.file emitted by module-deps is exactly the same value that it'll use in the cache lookup. It might trip up the filesystem system watcher if it's a relative path not from the cwd.

[jmm]

@zertosh thanks for the feedback.

However, worst case scenario, you have to add the plugins in a certain order - not unlike transforms.

Yeah, if the module-deps phase was labeled right now, I'd use that to be sure I'm inserting my stream directly after, although watchify or something else that comes after could do the same thing.

👍 - but is possible with labeled-stream-splicer to add new labels? Or would you have to re-create the whole pipeline?

Yes, you can just label the stream you're adding (or streams that are already in the pipeline). In this case it would just be a matter of:

b.pipeline.get('deps').push(through.obj(function(row, enc, next) {
...
}));
b.pipeline.get('deps').get(-1).label = 'watchify';

(I suggested a naming convention of {{pkg-name}}:whatever, but if a package is only forseeably adding one stream to the pipeline or a given pipeline phase, I guess just {{pkg-name}} is sufficient.)

Or you could assign the through.obj() return value to a var, set the label prop, and then push() it.

Then you could do b.pipeline.get('deps').get('watchify'). This is actually pushing the watchify stream to the deps sub-pipeline. I wonder if it would actually make more sense for this to be a top-level pipeline phase?

I think step 2 should've have been pushed after the json pipeline step. I think that'll cause double "module.exports=" to be prepended.

I was suggesting doing just one or the other: either transform just files with .avsc extension to prepend module.exports=, or lie about the file name before json phase. I updated my post to try to make that clearer.

Interesting. It shouldn't matter from a watchify perspective because the value of row.file emitted by module-deps is exactly the same value that it'll use in the cache lookup.

Ok, sorry for the noise.

[zertosh]

Ok, sorry for the noise.

Nah, I'm very happy that browserify and watchify are getting TLC 😄