Skip to content
This repository has been archived by the owner on Jul 8, 2020. It is now read-only.

watching files doesn't work #317

Closed
azachar opened this issue Feb 10, 2016 · 6 comments
Closed

watching files doesn't work #317

azachar opened this issue Feb 10, 2016 · 6 comments

Comments

@azachar
Copy link

azachar commented Feb 10, 2016

Hi there,

I would like to open a discussion regarding the gulp watch functionality.
I think that watching temporary files doesn't make sense and it brings more issues. Even a simple change of scss files might cause loosing injected css files so after some changes you need to stop and run gulp watch again.

Image if you change a scss file you would expect that
0. sass file is modified

  1. sass file is compiled into tmp/*/.css
  2. linting of sass file (nor the compiled css file where you have no control)
  3. file is injected into index.html
  4. browser is reloaded

In fact the watching function triggers this weird loop:
0. sass file is modified

  1. linting is involved on tmp css file --- seems to me a wrong. (now are supported only json and js files)
  2. all css files are removed before the compilation of new css file, this trigger an another watch function that starts removing injected css file, because they were removed from tmp folder, which is a source folder for injecting.
  3. sass compilation is done, the inject function is called again and the file is injected.

Is you can see there are same race conditions that might cause some unpredictable behaviour as we can see in #223

It seems to me that this is related to #129

I think the easiest way is to watch only source files (sass, jade what so ever) and if there is a need to inject them, the injection should be done in the corresponding task like styles. This will reduce race conditions. Also cleaning should be done more carefully. E.g. we should clean only files related to a change not to all of it.

What do you think? Do you see any problems with this approach?

Best regards,
Andrej

@gruppjo
Copy link
Contributor

gruppjo commented Feb 12, 2016

Hi Andrej, I'm planning to do a general refactoring of the gulp tasks with gulp 4.0 but for now it's just not worth the effort. The current collection of gulp tasks has grown historically and is thus not implemented in an optimal fashion. However they do work and serve their purpose, so the pain to gain ratio is just good.

Edit: accidentally closed it.

@gruppjo gruppjo closed this as completed Feb 12, 2016
@gruppjo gruppjo reopened this Feb 12, 2016
@azachar
Copy link
Author

azachar commented Feb 13, 2016

Hey Jonathan, thanks for the explanations, let's wait for gulp 4.0 :)

@nirus
Copy link

nirus commented May 26, 2016

Hi,

First of all thank's a ton for this generator. Big shout out to all the contributors.

Well speaking on the issue, yes race condition is happening when ever i modify only SCSS file.

Flow:

SCSS Change -> Style task triggered -> Clean Task triggered -> SCSS Compiled -> Write to .tmp folder.

Culprit code for race bug in the above flow:(File - watching.js)

var watchFiles = paths.jsFiles
    .concat([
      'app/index.html',
      '.tmp/*/styles/*.css', /* This line creates a race condtion*/
      'app/*/assets/**/*'
    ])
    .concat(paths.templates);

Explaination: This is piece of code is triggering a watch task for .css which triggers linting task and loop goes on.

Solution/Fix

@azachar

Comment this code .tmp/*/styles/*.css in watching.js and everything works fine.


Question to Author

I am curious to know why a watch for .css file in .tmp folder is required.

Can i create a pull request to fix the issue?

Hope this helps solving this issue. Keep up the good work. Appreciate the effort.

@gruppjo
Copy link
Contributor

gruppjo commented Jun 1, 2016

Hi @nirus, thanks for your valuable input.

.tmp//styles/.css is necessary

Watching .tmp/*/styles/*.css is necessary for triggering the browser reload as it's not possible to trigger it after styles has finished. Gulp doesn't provide a signature with dependent tasks AND a callback, which would definitely help to implement this more cleanly:

// WON'T WORK
gulp.watch('app/*/styles/**/*.scss', ['styles'], function () {
   bs.reload();
});

Alternative

However deleting the css shouldn't cause the any other tasks to start. So I came up with this version of the watch tasks that watches for css changes separately and only triggers a browser reload. Can some of you verify if this helps? If so, I'll include it in the next release, maybe even publish a bugfix version.

watching.js starting from line 39

// WATCH
gulp.task('watch', ['inject-all'], function () {

  // browser sync server
  bsInit(['app', '.tmp']);

  var watchFiles = paths.jsFiles
    .concat([
      'app/index.html',
      'app/*/assets/**/*'
    ])
    .concat(paths.templates);

  // start linting and watching
  gulp.start('linting');
  gulp.watch(watchFiles, function (event) {
    console.log('File ' + event.path + ' was ' + event.type + ', running tasks...');
    if (event.type === 'changed') {
      bs.reload();
      gulp.start('linting');
    }
    else { // added or deleted
      // inject in index (implicitly reloads)
      gulp.start('inject-all');
    }
  });
  // watch for changes in scss
  gulp.watch('app/*/styles/**/*.scss', ['styles']);
  // watch for changes in css
  gulp.watch('.tmp/*/styles/*.css', function () {
    bs.reload();
  });
  // watch for changes in environment files and new config files
  gulp.watch([
    'app/main/constants/env-*.json',
    'app/*/constants/*config-const.js'
  ], ['environment']);
});

Please, let me know if this works for you!

@cherylhanlon
Copy link

I have tested this and it seems to work! Thanks.

@gruppjo gruppjo added this to the 1.9.0 milestone Jun 6, 2016
@gruppjo gruppjo closed this as completed Jun 22, 2016
@gruppjo
Copy link
Contributor

gruppjo commented Jun 22, 2016

for future reference, this could prevent multiple browser reloads, when css watch is triggered with multiple modules:

// watch for changes in css
  gulp.watch('.tmp/*/styles/*.css', (function () {
    var last = 0;
    var tolerance = 15; // don't retrigger within this tolerance
    var lastTimeout;
    return function () {
      var now = new Date().getTime();
      var delta = now - last;
      if (delta < tolerance) {
        clearTimeout(lastTimeout);
      }
      last = now;
      lastTimeout = setTimeout(function () {
        bs.reload();
      }, tolerance);
    };
  })());

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

No branches or pull requests

4 participants