Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gaze.prototype._addToWatched repeating readDir of same directory on each call #214

Open
smithwib opened this issue Mar 18, 2016 · 6 comments

Comments

@smithwib
Copy link

Thanks for the module - super cool.

We're running into a delay when a project site got past a couple dozen files and headed into the hundreds, we noticed the processing time was taking longer and longer -- minutes. In debugging, I focused in on this:

    var dirname = (helper.isDir(file)) ? filepath : path.dirname(filepath);
    dirname = helper.markDir(dirname);
..
    var readdir = fs.readdirSync(dirname);
    for (var j = 0; j < readdir.length; j++) {
      var dirfile = path.join(dirname, readdir[j]);
      if (fs.lstatSync(dirfile).isDirectory()) {
        helper.objectPush(this._watched, dirname, dirfile + path.sep);
      }
    }  

When supplied a glob like ./data/data.json, it set dirname = ./data (the parent folder), and then it readdirSync that directory and added its subdirectories to the watchlist.

Two strange things:

  • I'm not sure why it would push the subdirectories, since I was just looking for one file
  • When you process 100 files in the same folder, it is doing the same thing over and over -- readdirSync the parent folder. That seems unnecessary.

Can you help?

@smithwib
Copy link
Author

This is what it seems it should be, but I'm not familiar with all use cases:

// Adds files and dirs to watched
Gaze.prototype._addToWatched = function(files) {
  for (var i = 0; i < files.length; i++) {
    var file = files[i];  
    var filepath = path.resolve(this.options.cwd, file);

    var dirname = (helper.isDir(file)) ? filepath : path.dirname(filepath);
    dirname = helper.markDir(dirname);

    // If a new dir is added
    if (helper.isDir(file) && !(filepath in this._watched)) {    
      helper.objectPush(this._watched, filepath, []);

      // add folders into the mix
      var readdir = fs.readdirSync(dirname);
      for (var j = 0; j < readdir.length; j++) {
        var dirfile = path.join(dirname, readdir[j]);
        if (fs.lstatSync(dirfile).isDirectory()) {
          helper.objectPush(this._watched, dirname, dirfile + path.sep);
        }
      }       
    }

    if (file.slice(-1) === '/') { filepath += path.sep; }
    helper.objectPush(this._watched, path.dirname(filepath) + path.sep, filepath);

  }
  return this;
};

@shama
Copy link
Owner

shama commented Mar 19, 2016

Hey! Thanks for diving into this. As long as all the tests pass with npm test all the use cases we support should be good. Feel free to open a PR! Thanks!

@smithwib
Copy link
Author

My pleasure, but unfortunately I ran npm test and did see a slew of errors like below. I fear it may be because I am on a PC with reduced admin rights, a dated version of node (0.12.7), and am behind a firewall, (aka developer heaven). Example:

Error: 'added.js' == 'Project (LO)'
at Object.equal (C:\wamp\www\gaze\node_modules\grunt-contrib-nodeunit\node_mo
dules\nodeunit\lib\types.js:83:39)
at Gaze. (C:\wamp\www\gaze\test\patterns_test.js:30:14)
at Gaze.emit (events.js:107:17)
at Gaze.emit (C:\wamp\www\gaze\lib\gaze.js:128:32)
at Gaze. (C:\wamp\www\gaze\lib\gaze.js:401:18)
at null._onTimeout (C:\wamp\www\gaze\lib\gaze.js:429:24)
at Timer.listOnTimeout (timers.js:119:15)

Should I proceed with the PR and lean on you to do a real npm test?

chebum pushed a commit to chebum/gaze that referenced this issue Jun 28, 2016
@chebum
Copy link

chebum commented Jun 28, 2016

Hi,

I ran into the same issue as @smithwib. Making a fix, I doubt if the test is correct. This line bothers me:
var expected = { '.': ['Project (LO)/', 'nested/', 'one.js', 'sub/'], };

The problem I see here is that we didn't add sub dir into the watch list. The test adds folders Project (LO) and nested only.

This may have unpleasant side-effects. For example, gaze is used in metalsmith-watch library. Metalsmith is a static website generator written in Node.JS. Common folder structure is

  • root
    • src // pages
    • build // ready website
    • node_modules

You configure to watch for changes in src folder and gaze also starts monitoring node_modules folder as well. Is it an expected behavior?

@shama Kyle, what do you think?

@ghost
Copy link

ghost commented Nov 14, 2016

Has this been solved?

@paulbouwer
Copy link

Is there a reason that sub folders are scanned and added to the watch list when a single file is specified to be watched and not a file glob that could potentially include sub-folders? This is causing issues in hyper where only a single file should be watched.

I added the following issue in the hyper repo:

There are a number of issues being raised in hyper that are directly related to the fact that gaze is scanning sub-folders in a users HOME when a single file (.hyper.js) is added to be watched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants