Make `args` a function, to prevent inadvertent sharing of caches across instances

[jmm]

Isn't this...

module.exports.args = {
    cache: {}, packageCache: {}
};

...and this...

You can also just do:

var b = browserify(watchify.args);

...a bad idea to naively encourage? A la @zertosh https://github.com/substack/watchify/issues/187#issuecomment-89688765.

If so, this PR would change the usage to:

var b = browserify(watchify.args());

If this change moves forward there are tests to fix, docs to update etc. I decided to stop here until I see what you think about the idea in general, and about backcompat if you're favorable.

• Preserve partial BC by making args a getter.
• Preserve full BC by making a new export, e.g. getArgs().
• Break BC.

Exporting a function would also make it simple to provide the following convenience if you want to:

// Merge passed opts with the watchify args.
watchify.args({debug: true, ...})

[zertosh]

@jmm This is a major change. I think it's a great idea, as you've seen from my comments. Besides the things you've mentioned, we also need to update the docs in gulp/SO.

Since this will be a major version bump, we should lump together a few breaking changes. I have an idea for a better https://github.com/substack/watchify/pull/199. I'll try it in a few days - def by the weekend.

I also want to add https://github.com/substack/watchify/pull/197, but it requires a bit for coordination with browserify. I'll make a PR there, but basically, browserify should switch from v to V for the version, so there are consistent flags across the two.

[zertosh]

Actually, since fullPaths is now optional, I wonder if the default should be to create a new cache per instance, unless you explicitly opt out by passing {cache: false} (or if you pass in {cache: myCache}, we'll use that instead). That way, we can remove watchify.args and whoever is using it but upgrades, won't be affected since it'll just be undefined.

[jmm]

The concerns about sharing cache across instances probably don't apply to sharing packageCache I'm thinking?

[zertosh]

@jmm Nope, they don't. But that's so minimal, that I'd rather not introduce more complexity and another point of failure by sharing packageCache but not cache.

[jmm]

Re: #199, on the browserify side that is a complicated problem. I'm looking into that, interwoven with some other threads. For example, I'm trying to put together some documentation for the row props through the pipeline, and while looking into some watchify / plugin interaction issues with my documentation project I noticed that it looks like there's a cache miss on dependencies of entry files because if you do: browserify('./whatever'), then when module-deps checks the cache for deps of whatever it's looking up with the absolute path, but the entry is cache['./whatever'].

So I don't know how that'll play out. If it's causing big trouble here perhaps it makes sense to duct tape it for now and revisit it if and when it gets sorted out in browserify.

Re: #197 -- sounds like you've changed your mind on that since your last comment? FWIW, most of the linux executables I use that have a short version option seem to use -v, although I often just do --version because there are enough of them that don't. If watchify is getting a major version bump, it wouldn't make more sense to change watchify's -v to be version, and make -V (or whatever) verbose? In any case, I focus on the JS API much more than the CLI. (I'm actually working on something that I hope might marginalize the CLI.)

@jmm Nope, they don't. But that's so minimal, that I'd rather not introduce more complexity and another point of failure by sharing packageCache but not cache.

Makes sense. So yeah, do you think not wanting to share the cache is indeed the most common use case? If so then maybe simplifying that even further and requiring extra work to opt-in to sharing is the way to go. So the assumption with #160 was that people wouldn't want fullPaths by default once they weren't required to use it?

[mattdesl]

If watchify is getting a major version bump, it wouldn't make more sense to change watchify's -v to be version, and make -V (or whatever) verbose?

Worth noting: subtle changes like this can create headaches for libraries and tools already built on watchify (e.g. budo). It also causes problems for documentation, tutorials, etc that have been built up over time. Overall I would question whether its worth the change, since it seems purely aesthetic.

The cache stuff is a more reasonable breaking change, and it's something I can detect in order to continue supporting multiple versions of watchify in my libraries. :smile:

[jmm]

@mattdesl Just to be clear, I'm not advocating changing the meaning of watchify's -[vV] per se, but if I understood @zertosh correctly he's going to propose changing the meaning of browserify's -[vV] to align with watchify. All I'm saying is if one is to change to match the other, would it make more sense to change watchify's or browserify's?

[mattdesl]

If a change is necessary, I agree it would make more sense to change watchify to -V. Mainly I am questioning whether a change is needed; it will break everybody's current npm scripts as well as any tools that wrap watchify (budo, amok, ReactLiveReload, etc).

It's not a big deal for me since I am already aware of this issue, but most users are not subscribed to watchify's changes and so this error will just creep up on them one day and cause their build to break, and force them to spend time debugging why it's no longer working. Small things like this tend to frustrate users who don't really care whether or not the tool supports a short form for --version.

Just my 2c :smile:

[zertosh]

@jmm

If it's causing big trouble here perhaps it makes sense to duct tape it

Yup. When the row biz is settled we can coordinate a simultaneous major version bump for browserify and watchify to undo it.

Re: #197 -- sounds like you've changed your mind on that since your last comment?

Yes/no. rsync, ssh, and cp (just to name a few) use -v/--verbose like watchify does. And grep uses -V for version. My guess is that nobody uses browserify's version flag, but everyone uses watchify's verbose flag - that's why I suggested it change in browserify - less breakage (as pointed out by @mattdesl). Another option would be for browserify to drop -v and only use --version. I'm aiming for consistency.

Also, I think there's little value in adding a flag just for the watchify version. However, if it also showed the browserify version available to watchify and their respective paths, then that'd make it a good troubleshooting tool. A lot of issues could be easily cleared up if users could tell right away if they're using global/local versions and what they are.

In any case, I focus on the JS API much more than the CLI. (I'm actually working on something that I hope might marginalize the CLI.)

Nice! Looking forward to seeing it. I personally never use the CLI, but most of the reported issues are from users that use the CLI, so it must be important to some.

do you think not wanting to share the cache is indeed the most common use case?

Yup. I think that the use cases for sharing a cache are rather advanced, and that would only work via the API anyway - so it makes sense for it to be opt-in. I hated being forced to use fullPaths. It crowds the source maps and stack traces. It doesn't let bundle-collapser do it's thing (one more thing to remember to turn off when you're making production builds). Different devs can't make deterministic builds. So yeah, forcing fullPaths is not cool.

[jmm]

@zertosh

Yup. When the row biz is settled we can coordinate a simultaneous major version bump for browserify and watchify to undo it.

Sounds good.

My guess is that nobody uses browserify's version flag, but everyone uses watchify's verbose flag - that's why I suggested it change in browserify - less breakage ... Also, I think there's little value in adding a flag just for the watchify version. However, if it also showed the browserify version available to watchify and their respective paths, then that'd make it a good troubleshooting tool. A lot of issues could be easily cleared up if users could tell right away if they're using global/local versions and what they are.

Ah, got it. Thanks for cluing me in. I do sometimes use browserify's version flag (like today), but to me personally it's no big deal if it's --version instead of a short opt.

Nice! Looking forward to seeing it.

Thanks! I'll ping you when there's something to see.

but most of the reported issues are from users that use the CLI, so it must be important to some.

Yeah, I kind of don't get why it seems to be so widely used.

Yup. I think that the use cases for sharing a cache are rather advanced, and that would only work via the API anyway - so it makes sense for it to be opt-in.

In that case your idea of just getting rid of watchify.args sounds like a win.

I hated being forced to use fullPaths. It crowds the source maps and stack traces.

I don't have much experience with it. I'm generally trying to require() module style (non-relative, non-absolute) paths and not have absolute local paths exposed in the bundle or source maps. My approach might interfere with bundle-collaperser too -- haven't used it.

Different devs can't make deterministic builds.

I'm probably overlooking something obvious, but why is that? With fullPaths aren't the paths in the bundle relative to the basedir and can't you enforce consistency with that across build environments by explicitly setting it? Or is it a cross-platform issue?

[zertosh]

With fullPaths aren't the paths in the bundle relative to the basedir

fullPaths makes the paths absolute from the system root - for the purposes of source maps and most importantly, for the deps and module id used by browser-pack. Not setting fullPaths produces source maps with paths relative to the basedir, and deps relative to the module that required them. So, since it's highly unlikely that two devs would keep the same project in the same subdirectory, building with fullPaths will yield two slightly different builds. (#confession: I have no idea what happens in windows or what paths look like)

[jmm]

@zertosh

fullPaths makes the paths absolute from the system root - for the purposes of source maps and most importantly, for the deps and module id used by browser-pack. Not setting fullPaths produces source maps with paths relative to the basedir, and deps relative to the module that required them.

Ah, yeah, my bad. That's weird that with fullPaths entry file paths are relative (if they were specified to browserify that way) but dep file paths are absolute (even if require()'d with a relative path). Perhaps related to the inconsistencies in the handling of the row props.

So, since it's highly unlikely that two devs would keep the same project in the same subdirectory, building with fullPaths will yield two slightly different builds.

Yeah, I see what you mean. And like I said, I generally don't want absolute paths exposed anyway.

Re:

I'm actually working on something that I hope might marginalize the CLI.

Check it out if you want. I have to write a blog post / documentation to explain it, but you'll get the gist of it: pseudo-JS with a compact, CLI-like version of the API that reduces the shell part of the operation to passing a string of pseudo-JS to a script. So "CLI" commands mostly written in almost-JS.

[jmm]

@zertosh re: https://github.com/substack/watchify/issues/207#issuecomment-94839035

Here are a few ideas for your consideration:

1. Make watchify.args a getter that returns a new instance of {cache: {}, packageCache: {} each time. As I noted above this is partially preserving BC, but based on the discussion here it sounds like the BC it would break should be broken.
2. Offer as a convenience to construct a browserify instance for the user:

// Set `cache` and `packageCache` on the browserify opts
// if `undefined`
var w = watchify({debug: true, ...}, {delay: 600, ...});

Stipulate that otherwise they have to construct the browserify instance with the necessary args.

3.

But we need some form of watchify.args that creates new cache objects - like your PR. Though, that kinda sucks because if someone keeps using watchify.args, it won't break - it'll just silently not cache.

You could do what you suggested in https://github.com/substack/watchify/issues/207#issuecomment-94839035, or, since watchify is already reading b._options you could throw if cache / packageCache is undefined so it won't silently not cache (and like you said here require passing false to opt out).

---

FWIW, here are my opinions (but you know more about watchify usage than I do): I think the current incarnation of watchify.args is clunky, not even counting the sharing issue. I think a watchify.args(opts) that merges the passed opts with the default ones is more useful. But, for common use cases I think perhaps the approach in # 2 makes the most sense. I think it might still make sense to keep some incarnation of watchify.args around, but promote # 2 as the preferred way to do it unless there's a good reason not to.

[mattdesl]

Or:

Leave args as-is (clunky) and mark it as deprecated (either in docs or with a stderr warning). Then use another function like defaults() going forward.

[zertosh]

@jmm

Offer as a convenience to construct a browserify instance for the user:

It was like that before v1.0.0 (e.g. v0.10.2). The way it is now, watchify behaves like a true plugin (with a leaky cache implementation).

I have to think this through, but I like what bundle-collapser does. It's basically broken up into three parts: cmd.js, index.js, and plugin.js - where plugin.js is analogous to watchify's index.js, and its index.js is analogous to a watchify that constructs a browserify with a cache. Just a thought.

I think the current incarnation of watchify.args is clunky, not even counting the sharing issue.

:+1: ^ 100

@mattdesl

Leave args as-is (clunky) and mark it as deprecated (either in docs or with a stderr warning). Then use another function like defaults() going forward.

This is the simplest short-term solution, but I don't know if it's the best. I'm worried about developer fatigue with a moving API.

---

@jmm

What if Browserify where the one to set cache and packageCache on the options at construction time if they're undefined? And watchify simply populates them if they're available? cache would be a noop if watchify isn't plugged in. But packageCache, when set, gets automatically populated by module-deps - yeah weird. I don't know how we'd deal with this.

---

In any case, sorry for these long discussions and my absenteeism this week. I'm recovering from some stomach problems. But I'll put some further thought into this now.

[jmm]

@zertosh

I'm recovering from some stomach problems. But I'll put some further thought into this now.

I'll reply to the other stuff later but just saw this and wanted to say sorry to hear about the stomach problems. Hope it's nothing serious and you're feeling better soon.

In any case, sorry for these long discussions and my absenteeism this week.

No worries here. I have no problem discussing stuff until it's sorted out. And regarding your level of availability -- so it goes with this stuff, ebb and flow. Even when you decide to work on watchify stuff, I'm sure this issue isn't your highest priority (and I was taking a cue from you when I did this anyway).

[zertosh]

Thanks @jmm :) I should be fully recovered in 2-3 weeks, but I'm feeling much much better now.

What do you and @mattdesl think about https://github.com/substack/watchify/pull/213?

[jmm]

@zertosh

I should be fully recovered in 2-3 weeks, but I'm feeling much much better now.

Glad to hear that!

[jmm]

@zertosh

What if Browserify where the one to set cache and packageCache on the options at construction time if they're undefined? And watchify simply populates them if they're available? cache would be a noop if watchify isn't plugged in. But packageCache, when set, gets automatically populated by module-deps - yeah weird. I don't know how we'd deal with this.

Did you decide against this idea or do you still think it's worth looking into? I don't know all of the ramifications off the top of my head (I was actually thinking recently about taking a closer look at how caching works in module-deps for unreleated reasons -- it sounds like you may be ahead of me in understanding that).

[zertosh]

Did you decide against this idea or do you still think it's worth looking into?

I'll look into again this weekend. Since cache is a noop if it's not populated, that's fine. But packageCache would involve not caching in module-deps, and instead cache it in watchify. This would be the ideal solution and I think it would fix https://github.com/substack/module-deps/pull/81.

it sounds like you may be ahead of me in understanding that

Nah! Don't say that. All you need is small 3 file project (this one is good https://github.com/substack/watchify/issues/177#issuecomment-92689923) and some patience with node-inspector :smile:

[jmm]

Nah! Don't say that. All you need is small 3 file project (this one is good #177 (comment)) and some patience with node-inspector :smile:

Oh totally! I've looked at module-deps quite a bit, including with node-inspector (which is awesome). But I haven't looked at what it does with the cache properties much, so I expect you know more about what happens with those than I do right now.

Speaking of #177, I was interested in what you did with substack/module-deps#80, because the day before I was musing about the idea of supporting in-memory files (like vinyl) better. I was thinking it'd be cool if you could feed a set of such files into browserify and have it resolve deps in those and other files to the files already in memory, which would require holding up deps resolution until the entry files are accounted for and put into some sort of temporary cache. So it looked like your PR did the first part of that anyway. That's the other reason I was interested in looking into the caching behavior more.

[zertosh]

I removed the reference to watchify.args from the readme, and added a few more examples that should drive the point home (see e8f0217).