update BrowserSync #1457
update BrowserSync #1457
Conversation
|
This reloads javascript twice due to the fact that the .map file gets through. Have the pattern exclude .map files
edit: wrong screenshot lol edit again: well actually the map files show up as changed in the previous method as well
But the javascript is still injected twice in this PR and only once in master |
|
lol @austinpray |
|
I was just about to post a question about that over on Discourse... I think it's actually a problem here:
If you remove |
|
please stop making unnecessary comments on github, especially emoji ones. lots of us get email notifications for each comment. thanks! |
|
The extra 'File changed' notice likely results from a change in BrowserSync's matcher. (This would then affect #1448 as well.) As noted above, line 239 now seems be recursively reading Checking against master with only BrowserSync updated, any file changed inside Rolling back to BrowserSync v2.5, nothing happens when touching |
| .pipe(function() { | ||
| return gulpif('**/*.{js,css}', browserSync.reload({stream:true})); | ||
| }) | ||
| .pipe(browserSync.stream, {match: '**/*.{js,css}'}) |
There was a problem hiding this comment.
Actually just remove this. With the new version the path.dist in files is enough to make things work.
Everything works exactly as expected when you remove this line.
There was a problem hiding this comment.
seriously? I'll try that too.
There was a problem hiding this comment.
Yep it reloads images and such too.
There was a problem hiding this comment.
I don't think that really works. I'm seeing it picking up every changed file in dist, including source maps. The additional files mess up BrowserSync's asset-injection and force a full page reload. With a lot of files, they can also send multiple reload triggers.
There was a problem hiding this comment.
JavaScript files should be a full page reload. CSS injecting is working properly over here after omitting the inline reload.
There was a problem hiding this comment.
Can you compare my bs_files-only branch with the code you're running? I'm only seeing full reloads from that, no CSS injection and a ton of changed file notices.
|
I'm testing two solutions:
I think the second option is probably best. |
|
Option 1 works great, what's wrong with that? I use option 1 more frequently than all this stream stuff. edit: sorry was confused. Neither of those options are necessary. |
austinpray
changed the title
See #1457 (comment) This is option 2.
|
Using BrowserSync's stream support means there's no need to watch output files for changes-- the output files' contents are already being piped through the stream. This saves the overhead of additional file watches and reading back the same file we just wrote from the task. It doesn't make sense to watch a stream's output file for changes when the stream itself is available. I think this should be nailed down now. Two other branches were pushed to my fork to isolate the solutions described above for testing: bs_path-dist-mod and bs_files-only. |
|
I just tested out https://github.com/joemaller/sage/tree/bs_path-dist-mod Yeah let's definitely do what you have currently on the PR. Works perfectly. Also you are right: the less files we watch the better. Especially on Windows. |
| @@ -198,7 +196,8 @@ gulp.task('scripts', ['jshint'], function() { | |||
| gulp.task('fonts', function() { | |||
| return gulp.src(globs.fonts) | |||
| .pipe(flatten()) | |||
| .pipe(gulp.dest(path.dist + 'fonts')); | |||
| .pipe(gulp.dest(path.dist + 'fonts')) | |||
| .pipe(browserSync.stream()); | |||
This PR switches to BrowserSync's recommended "post 2.0.0 syntax" and uses the cleaner
streammethod for reloading. The stream method offers glob filtering, so we are able to simplify the reload pipe inwriteToManifesta bit.This is compatible with #1448.