browserify / watchify

watch mode for browserify builds
Other
1.79k stars 181 forks source link

`watchify` does not create watchers if a previous used `cache` object is passed from a previous run #240

Closed royriojas closed 9 years ago

royriojas commented 9 years ago

I have worked on a module called bundly that provides a cache option for watchify builds. This is useful for scenarios where the watch mode is not available or desirable but still wants to reuse the previous result of transforms of files that didn't change. For example on a building server or to speed up initial builds when entering the watch mode.

This module is working pretty well with watchify@2.6.x, but the newer versions remove the call to add the watchers in the collect function which breaks the functionality of this module during watch mode when using the cache object from a previous run.

this repo can be used to replicate the issue https://github.com/royriojas/bundly-usage-demo/tree/using-watchify-latest

This branch is using an npm-shrinkwrap.json file which is used to force bundly to use the latest version of watchify (3.2.3) in order to demonstrate the bug when persisting the watchify cache

To fix it:

The master branch uses the previous version of watchify (2.6.x) and it works just fine.

https://github.com/royriojas/bundly-usage-demo/tree/master

royriojas commented 9 years ago

@zertosh,@substack I have made a wrapper around watchify, but I have to fork watchify to include the fix that I show in this bug, any chances you guys can consider adding this line back?

https://www.npmjs.com/package/persistify

It is just a wrapper over browserify or watchify to provide a persistent cache between executions of the command, can be used interchangeably with watchify or browserify

If you install the module, and install watchify from the official repo you will see the update events are not fired, because the file event are not fired if the cache already exist.

Please let me know, for now my fork works, but it would be nice to use the official one.

zertosh commented 9 years ago

@royriojas I tried to look at your demo but it's too complicated. I was hoping for something that highlighted why your implementation explicitly required the watchFile call where you wanted it in watchify.

For what watchify does, the call to watchFile isn't needed in collect. Doing so, would be adding overhead for no reason. If your code is flexible as to when watchFile is called, you can very easily just "trick" watchify into calling it (no promises on that this will work in the future):

b.on('reset', emitFile);
emitFile();

function emitFile () {
    b.pipeline.get('deps').push(through.obj(function(row, enc, next) {
        b.emit('file', row.expose ? b._expose[row.id] : row.file);
        this.push(row);
        next();
    }));
}

That should work because watchify calls watchFile whenever the browserify instance emits a file event.

I'm still open to helping you find a way to implement your cacher w/o having to this, but I need to see your implementation w/o any cruft.

royriojas commented 9 years ago

@zertosh thanks for your time man, what about adding a watchFile method on the returned bundle? you're already exposing the close method, so you might also expose a watchFile/watch method so files can be added from outside to the watch list

I will try your suggestion as it seems it might work as well.

Again, thanks for your time

royriojas commented 9 years ago

@zertosh, by the way, did you check this? https://github.com/royriojas/persistify/blob/master/index.js, this is less convoluted than my original attempt, and less ambitious, it just wraps watchify or browerify to provide the persitent cache. The orignal demo in the post is in fact convoluted, but this one is a lot simpler.

zertosh commented 9 years ago

@royriojas: I'm still not getting it – I'm really sorry. I looked through persistify and I'm not exactly sure why you depend on the watchFile call. In any case, to help you make progress on this, I wrote out the skeleton of how I would implement persistent cache. It's meant to be used a browserify plugin, and it doesn't depend on watchify (but it respects it if it's running). I hope this helps:

module.exports = function apply(b, opts) {
  var mdeps = b.pipeline.get('deps').get(0);

  // make sure we don't blow away an existing cache object,
  // it might be watchify's.
  if (!mdeps.cache) mdeps.cache = {};
  var cache = mdeps.cache;

  // pause the "record" stream so that on "bundle" we can
  // verify the freshess of the cache before module-deps starts  
  var record = b.pipeline.get('record');
  record.pause();

  b.once('bundle', function() {
    // HERE: verify the freshness of the cache. delete any
    // files that are stale - watchify may have done this already
    // but we don't know if the user is using watchify.
    // Or populate it if it's empty because this is the first run.

    // HERE: once you're done, resume the "record" stream
    // (fyi: this can done async)
    record.resume();
  });

  // make a copy of the module-deps output
  var mirror = {};
  b.pipeline.get('deps').push(through.obj(function (row, enc, next) {
    var file = row.expose ? b._expose[row.id] : row.file;
    mirror[file] = row;
    this.push(row);
    next();
  }));

  // needsStat could be used as a hint for future runs
  // that we don't need to stat for file changes because
  // something else is updating the cache (like watchify).
  var needsStat = false;

  b.pipeline.get('deps').on('end', function() {
    // if watchify is running, then it would've synced the cache.
    // that is, it would've added/deleted/changed files as needed.
    // if watchify isn't running, then we have to sync the cache ourselves –
    // because there isn't a reliable way to know if watchify is running, or
    // if it's in "cache" mode.

    // this shouldn't be done in the above pipeline tap because
    // we don't know if watchify was added before or after us -
    // it's better to wait until "deps" is done
    Object.keys(cache).forEach(function(file) {
      if (!mirror[file]) {
        needsStat = true;
        delete cache[file];
      }
    });

    Object.keys(mirror).forEach(function(file) {
      if (!cache[file]) cache[file] = {};
      if (cache[file].source !== mirror[file].source) {
        needsStat = true;
        cache[file] = {
          source: row.source,
          deps: xtend(row.deps)
        };
      }
    });
  });

  b.pipeline.on('end', function() {
    // HERE: persist the cache to disk or wherever
  });

  // re-apply the plugin so it works on subsequent "b.bundle()" calls
  b.once('reset', function() {
    apply(b, opts)
  });
};
royriojas commented 9 years ago

no problem, @zertosh, the main issue is that if a file is already in the cache when the process starts it won't generate the file event, if the file event is not generated then no watcher is added for that file. I will just take a look if manually firing the file event solves my issue. I also tried to make it a browserify plugin first, but I lack the expertise with the internal pipeline. I will try to manually fire the event, I know that it might change in the future, but I hope it will not.

Regards

royriojas commented 9 years ago

by the way, @zertosh, thank you for taking the time for checking this. I like the idea of the plugin better, will see if can use what you have make as an skeleton.