9.0.8 breaks 'dep' event

[jwalton]

Since 'dep' isn't documented in the browserify readme, maybe this was intentional, but it's causing grief for me, so I thought I'd raise it in case it wasn't. :)

In browserify 9.0.7, if I did:

b = browserify(opts);
b.on('dep', function(dep) {
    console.log(dep.id + " -- " + dep.file);
});

then I'd get output like:

backbone.layoutmanager -- /Users/jwalton/project/node_modules/backbone.layoutmanager/backbone.layoutmanager.js

In 9.0.8, I get:

backbone.layoutmanager -- backbone.layoutmanager

dep.file is not actually a file here. I'm using uber-watchify, which tries to fs.statSync(dep.file), which is obviously going to end in tears for all concerned.

[jwalton]

Looks like the 'deps' pipeline has the same problem:

b.pipeline.get('deps').push(through.obj(function(row, end, next) {
    console.log(row.id + ' -- ' + row.file);
    next();
}));

gives the same output for 9.0.7 and 9.0.8.

[jwalton]

This is going to break watchify, too, although less catastrophically. Watchify will end up watching the non-existent process.cwd() + '/backbone.layoutmanager' instead of the file it's supposed to be watching. If this file gets created it will cause a rebuild.

[jmm]

@jwalton Thanks for the report.

dep is mentioned in the readme in the pipeline section:

'emit-deps' - emit 'dep' event

#1202 is what changed. This is related to the issue I raised in #1162 -- row.file (or rec.file) properties don't have consistent values. Sometimes they're absolute paths and sometimes not (like when an object with a file prop is passed to b.add() or b.require()).

I mentioned that with respect to watchify in browserify/watchify#160 and @zertosh said it wouldn't matter in that case, but not sure if that's exactly the same situation you're talking about.

[jwalton]

@zertosh said it might trip up the file system watcher, and he's right: this line is going to try to watch row.file, and when row.file isn't a real file, it's going to end up watching something you don't expect it to. Assuming this only happens for modules loaded from node_modules, then 99% of the time this is fine, as your module is unlikely to change, and the incorrect file it's watching is unlikely to exist (although you could probably construct examples where terrible things would happen.) Of course this all goes out the window if you're using application-specific modules, since you probably want watchify to pick up changes to those files.

uber-watchify has essentially the same problem; it tries to cache files across executions, though, so it needs to check and see if the source file's timestamp has changed. Since row.file isn't a real file in some cases, it fails when it tries to stat the file. The problem is worse with uber-watchify - it's much less reasonable to assume modules in node_modules haven't changed across executions.

[jmm]

@jwalton Thanks for the additional info and helping me put @zertosh's comment in context.

[jwalton]

@jmm If there's anything else you need from me, don't hesitate to ask. :)

[jmm]

@jwalton OK, thanks. I probably won't be leading the charge on this issue, since I was already seeking clarification on how row.file is supposed to work, but hopefully our posts provide a good starting point for @substack or whoever, and I'll be keeping an eye on how it plays out. I might be able to take a more active role if I get new insights here or on #1162.

[ghost]

There's some test coverage for the dep event in test/dedupe-deps.js which is still working. It must be some secondary factor that is causing dep events not to fire.

[jwalton]

They are firing, it's just that the 'file' value has changed.
On Apr 9, 2015 5:33 PM, "James Halliday" notifications@github.com wrote:

There's some test coverage for the dep event in test/dedupe-deps.js which
is still working. It must be some secondary factor that is causing dep
events not to fire.

—
Reply to this email directly or view it on GitHub
#1203 (comment)
.

[zertosh]

@jmm @jwalton I thought about this for a bit, played around some and came up with browserify/watchify#199. It works but I don't like the implementation. It feels fragile - like any little upstream change could break it. Any thoughts?

[jmm]

@zertosh Thanks for doing that.

It feels fragile - like any little upstream change could break it. Any thoughts?

That's how I feel about the situation. I think it's important for the row props (like id, file, and expose) to have well defined, and ideally consistent, semantics and types throughout the pipeline or it's going to be whack a mole: make a change that fixes something in one place and breaks something else in another place. I've seen this happen a few times. Here are a couple of examples directly related to these row props:

• After some previous iterations by myself and others to fix certain problems, I submitted Resolve pathnames of exposed modules more efficiently. #1077 / Add opts.expose object to populate resolved pathnames for exposed modules module-deps#65, which fixed some of the problems, but left it so that b.require('./somefile') is broken (omits transforms on somefile). One of the changes here was setting row.expose = row.id in b.require(). That enabled module-deps to set resolved pathnames for exposed modules, but also made it treat some of them as built-in.

• Then there was Fix expose field for external modules and builtins #1202

  Fix expose field for external modules and builtins

  Added the 'file' field at row creation in require() for external modules and builtins.

  Which solved a problem and, I believe, introduced this problem.

Like you said, if the current situation is worked around in places like watchify it's going to be fragile -- maybe even breaking if and when the situation is legitimately "fixed" upstream. If there are problems with the way these props are handled in browserify and its core dependencies I think it would be ideal to fix them there, but it's not something I could undertake independently.

[terinjokes]

I think it's important for the row props (like id, file, and expose) to have well defined, and ideally consistent, semantics and types throughout the pipeline

I agree, and it probably sounds like something module-deps should be defining.

[jmm]

@terinjokes I think I get the gist of what you're saying in the sense that module-deps is kind of the focal point of the pipeline, but how do you see that translating into practical terms? It needs to be clear what props may or not be available, and what form / significance they have as input and output respectively, at each phase of the pipeline. So it seems to me that requires a significant degree of coordination between the implementation and documentation of browserify and core dependencies, and that would make things predictable both internally, and externally for things like watchify and plugins.

[ElNounch]

As the one who PRed the unintentional grief, let me step in. ;)
First, I can just but agree there is some lack of specifications and test cases. ^^

For the time being, may you, please, have a look at not-yet-PRs ElNounch@f0caf5e and ElNounch/module-deps@f0e4252 ?
I mixed them in Browserify 'version' git://github.com/ElNounch/node-browserify.git#module_field_proposal.
I don't see another way to keep the id play his role, and file only pointing real files without adding another field, sort of a breaking change.

[jmm]

@ElNounch Thanks for joining the discussion.

I think it's possible that creating new props will make sense, but I don't think we're there yet. Perhaps the type and meaning of the existing props can be standardized and will do the trick. I think the behavior of those props may already be broken enough that changing it for the better in somewhat incompatible ways might be a reasonable course to take. This is what I'm thinking in broad terms (not necessarily exhaustive):

• row.id should always be the equivalent of the argument to a require() call -- a string that should have the module resolution algorithm applied to it, if resolution is necessary.
• row.file should always be a string that represents an absolute pathname, perhaps a sham value for streams or other forms of in-memory rows.
• row.expose should always be the value that governs if and how the module is exposed. If it's a string, it's the name to expose the module with. It may make sense for this to be allowed to be boolean true at certain pipeline phases to indicate that the module should be exposed with some auto-generated value.

[ElNounch]

I think of browserify as a archiver making a browser-side filesystem, the id being the absolute path in this simulated filesystem, sort of chroot()'ed to basedir, such as

/home/elnounch/project1 / index.js
                        / js       /a.js
                                   /b.js

gives me ids /index.js, /js/a.js and /js/b.js.
Exceptions being the external modules, with id's inacessible relatively, being out of the tree by not having the initial slash. jquery\dist\jquery.min.js:jquery becoming jquery.
That was my 0.02€ ;)

[jwalton]

This makes perfect sense, but if I'm writing a plugin for the archiver, I might still need to know where jquery actually came from. ;)

[ElNounch]

Sure, that's the goal of file field, to be resolved to the reality (shortest absolute path in real filesystem) behind an id in a sort-of filesystem, or undefined for builtins. :p
I just feel id is more a virtual-id than what-the-user-provided.

[zertosh]

@jmm I like the spec. Don't forget row.source and row.deps. There's also row.entry - I'm not sure if that should be considered a "public" prop or an implementation detail of module-deps.

[jmm]

@ElNounch

I think of browserify as a archiver making a browser-side filesystem, the id being the absolute path in this simulated filesystem, sort of chroot()'ed to basedir, such as

What's an example of how that property would be useful to a plugin or pipeline stream?

That's quite different from how it works currently. Currently, depending on input provided to browserify and pipeline phase, id may be a relative path, an absolute path, an arbitrary string that may resolve to a path, an integer ID.

If file (or some other property) were consistently an absolute pathname (or a sham one), and browserify exposed a basedir, then you could get the value you want with path.relative(basedir, file). Perhaps as a convenience it would be useful to expose this value as its own property, but I'm not sure it's a substitute for the kind of id value I suggested. A plugin or pipeline stream may have an interest in how the user or bundled code requested a file (I have such a plugin), not just the relative path from browserify's basedir to the resolved module file.

@zertosh Thanks for the feedback.

Don't forget row.source and row.deps. There's also row.entry - I'm not sure if that should be considered a "public" prop or an implementation detail of module-deps.

Absolutely, there's probably other props that should be documented, I was just focusing on a few of particular interest / relevance. I'm not sure any of these props can be considered truly private -- I think something needs to be said about each, if only "rows may contain this prop -- don't delete or tamper with it". As for entry specifically, I guess that depends whether a plugin / stream should be allowed to add / convert a row to an entry (not that things are necessarily setup in a way that would allow that right now -- I'd have to poke around to find out).

[zertosh]

@jmm I've got a non-hacky fix now 😄 However, it depends on browserify/module-deps#80, which fixes a race condition that sometimes causes "expose" to go missing (as reported by browserify/watchify#177).

Edit: The fix it the same PR ref'd above browserify/watchify#199

[jmm]

I'm working on some documentation for the pipeline / row props, but in the meantime if anyone is interested I wrote a small utility to help show how rows flow through the pipeline by logging them after each pipeline phase: browserify-row-flow. Gives a quick overview of when / where rows|props are added or modified.