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

Double notification on file change: body{background:blue}\nbody{background:blue} #1152

Open
saper opened this issue Sep 17, 2015 · 5 comments

Comments

@saper
Copy link
Member

saper commented Sep 17, 2015

This is a follow up to #1150 which still does not fix the issues.

Our two tests (quoted below) sometimes (rarely) produce two lines of output. This looks like a double notification coming, from whatever reason. Maybe there is an interesting relation because we have both index.scss and index.sass in the directory.

I am going to mark those tests as flaky to avoid CI issues until we fix that.

  1) cli node-sass in.scss should watch the full scss dep tree for a single file (scss):

      Uncaught AssertionError: 'body{background:blue}\n\nbody{background:blue}' == 'body{background:blue}'
      + expected - actual

      -body{background:blue}
      -
       body{background:blue}

      at Socket.<anonymous> (test/cli.js:317:16)
      at readableAddChunk (_stream_readable.js:146:16)
      at Socket.Readable.push (_stream_readable.js:110:10)
      at Pipe.onread (net.js:523:20)

Test code:

    it('should watch the full scss dep tree for a single file (scss)', function(done) {
      var src = fixture('watching/index.scss');
      var foo = fixture('watching/white.scss');

      fs.writeFileSync(foo, '');

      var bin = spawn(cli, [
        '--output-style', 'compressed',
        '--watch', src
      ]);

      bin.stdout.setEncoding('utf8');
      bin.stdout.once('data', function(data) {
        assert.equal(data.trim(), 'body{background:blue}');
        bin.kill();
        done();
      });

      setTimeout(function() {
        fs.appendFileSync(foo, 'body{background:blue}\n');
      }, 500);
    });

    it('should watch the full sass dep tree for a single file (sass)', function(done) {
      var src = fixture('watching/index.sass');
      var foo = fixture('watching/bar.sass');

      fs.writeFileSync(foo, '');

      var bin = spawn(cli, [
        '--output-style', 'compressed',
        '--watch', src
      ]);

      bin.stdout.setEncoding('utf8');
      bin.stdout.once('data', function(data) {
        assert.equal(data.trim(), 'body{background:red}');
        bin.kill();
        done();
      });

      setTimeout(function() {
        fs.appendFileSync(foo, 'body\n\tbackground: red\n');
      }, 500);
    });
@saper saper added this to the next.patch milestone Sep 17, 2015
saper added a commit to saper/node-sass that referenced this issue Sep 17, 2015
Sometimes they generate double notification
on change:

      1) cli node-sass in.scss should watch the full scss dep tree for a single file (scss):

      Uncaught AssertionError: 'body{background:blue}\n\nbody{background:blue}' == 'body{background:blue}'
      + expected - actual

      -body{background:blue}
      -
       body{background:blue}

      at Socket.<anonymous> (test/cli.js:317:16)
      at readableAddChunk (_stream_readable.js:146:16)
      at Socket.Readable.push (_stream_readable.js:110:10)
      at Pipe.onread (net.js:523:20)

sass#1152
@saper
Copy link
Member Author

saper commented Sep 17, 2015

It takes around of 15 minutes of running a pared-down version of tests/cli.js in a loop to reproduce those as above:

time sh -c 'while node_modules/.bin/mocha test/xcli.js ; do true; done; '

@xzyfer
Copy link
Contributor

xzyfer commented Sep 21, 2015

This could be a sass-graph issue. I'll take a look.

@saper
Copy link
Member Author

saper commented Sep 21, 2015

I also feel there's something wrong in

node-sass/bin/node-sass

Lines 247 to 258 in f6b5fd2

gaze.on('changed', function(file) {
var files = [file];
graph.visitAncestors(file, function(parent) {
files.push(parent);
});
files.forEach(function(file) {
if (path.basename(file)[0] !== '_') {
renderFile(file, options, emitter);
}
});
});
}

I'd love to have this moved to lib/watch.js and be properly tested internally.

@xzyfer xzyfer removed this from the next.patch milestone Apr 19, 2016
@saper
Copy link
Member Author

saper commented Oct 17, 2019

This needs to be tested again with the current code.

@saper
Copy link
Member Author

saper commented Oct 21, 2019

Unfortunately, I was getting the same on 4.12.0 trying to debug #2755

@saper saper changed the title Double notification on file change Double notification on file change: body{background:blue}\nbody{background:blue} Oct 21, 2019
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
Don't truncate trailing 0s on integers when precision is 0
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

2 participants