BUGFIX #6679 - workaround for tiny-lr not reloading on empty files arguments #6690

jurecuhalev · 2017-01-16T21:21:10Z

In a situation when you revert failed compile attempt in broccoli-persistent-filter's processString,

// lib/tasks/server/livereload-server.js

didChange(results) {
...
      this.tree = new FSTree.fromEntries(this.getDirectoryEntries(results.directory), {sortAndExpand: true});
      files = previousTree.calculatePatch(this.tree)
        .filter(isNotRemoved)
        .filter(isNotDirectory)
        .map(relativePath)
        .filter(isNotSourceMapFile);

the result of files is [].

This gets passed to this.liveReloadServer().changed({ which doesn't trigger new websocket push, because there's nothing to notify clients about.

This PR works around this by catching the fact that there was a compilation error and forcing reload on next didChange.

I'm not sure if it's possible to write test for this as tiny-lr's changed doesn't return. I've got test written for the Error handling portion of the functionality.

This might be a too complicated approach, but I'm not familiar with whole system enough to see any other options. Because it doesn't different between reasons for errors, it also fixes #6242.

stefanpenner · 2017-01-16T22:04:24Z

Let me summarize to make sure I understand this correctly (correct me if I mis-understand):

In-order to recover post error, if no files have changed (since we just restored to the original current state) we must reload anyways, as we are still on the error screen.

(if so, this seems reasonable)

stefanpenner · 2017-01-16T22:05:03Z

lib/tasks/server/livereload-server.js

+      this._hasCompileError = true;
+      files = ['LiveReload due to compile error'];
+    } else if ( this._hasCompileError) {
+      this._hasCompileError= false;


missing whitespace between _hasCompileError and =

stefanpenner · 2017-01-16T22:08:16Z

lib/tasks/server/livereload-server.js

@@ -137,8 +137,15 @@ class LiveReloadServerTask extends Task {
    let previousTree = this.tree;
    let files;

+    if ( results.stack ) {


extra whitespace between ( and results.stack and );

stefanpenner · 2017-01-16T22:08:30Z

lib/tasks/server/livereload-server.js

+    if ( results.stack ) {
+      this._hasCompileError = true;
+      files = ['LiveReload due to compile error'];
+    } else if ( this._hasCompileError) {


unbalanced whitespace

jurecuhalev · 2017-01-17T20:04:12Z

@stefanpenner your understanding of the issue is correct

jurecuhalev · 2017-01-17T20:14:03Z

I've fixed whitespace problems and squashed commits into single one.

stefanpenner · 2017-01-17T22:11:31Z

@gandalfar thanks!

This looks reasonable, but we likely need a unit test covering it. If you need help, let me know. We can likely lend a hand if needed.

jurecuhalev · 2017-01-17T22:30:53Z

Can you suggest what should be part of the test? I don't think I can capture tiny-lr output. I can assert against error stack trace and this._hasError.

stefanpenner · 2017-01-17T22:32:37Z

likely just ensure https://github.com/ember-cli/ember-cli/pull/6690/files#diff-60cfa3bc4236a25e0793052851917a47R164 gets invoked with the expected value.

jurecuhalev · 2017-01-18T20:29:51Z

I've added test that checks for changedCount and subject._hasCompileError. To actually track files contents, I'd have to modify

let stubbedChanged = function() {
      changedCount += 1;
    };

with custom changed method that tracks the value. I can do that if this is better approach (I was just looking at how other tests are written).

jurecuhalev · 2017-01-30T19:21:07Z

I've squashed commits and made sure it still applies to current master. Is there anything else I can do?

stefanpenner · 2017-01-30T19:22:15Z

@gandalfar this looks great! Sorry for letting this sit.

stefanpenner · 2017-01-30T19:22:41Z

re-running appvevor, will merge when green.

…files arguments

Turbo87 · 2017-02-10T14:19:23Z

finally 🍏 !

@homu r+

homu · 2017-02-10T14:19:25Z

📌 Commit 503ede1 has been approved by Turbo87

homu · 2017-02-10T14:19:39Z

⌛ Testing commit 503ede1 with merge 352d6b1...

BUGFIX #6679 - workaround for tiny-lr not reloading on empty files arguments In a situation when you revert failed compile attempt in `broccoli-persistent-filter`'s `processString`, ``` // lib/tasks/server/livereload-server.js didChange(results) { ... this.tree = new FSTree.fromEntries(this.getDirectoryEntries(results.directory), {sortAndExpand: true}); files = previousTree.calculatePatch(this.tree) .filter(isNotRemoved) .filter(isNotDirectory) .map(relativePath) .filter(isNotSourceMapFile); ``` the result of `files` is `[]`. This gets passed to `this.liveReloadServer().changed({` which doesn't trigger new websocket push, because there's nothing to notify clients about. This PR works around this by catching the fact that there was a compilation error and forcing reload on next `didChange`. I'm not sure if it's possible to write test for this as `tiny-lr`'s [`changed`](https://github.com/mklabs/tiny-lr/blob/master/lib/server.js#L220) doesn't return. I've got test written for the Error handling portion of the functionality. This might be a too complicated approach, but I'm not familiar with whole system enough to see any other options. Because it doesn't different between reasons for errors, it also fixes #6242.

homu · 2017-02-10T14:28:59Z

☀️ Test successful - status

stefanpenner requested changes Jan 16, 2017

View reviewed changes

jurecuhalev force-pushed the patch-livereload-6679 branch from f197755 to 147f599 Compare January 17, 2017 20:12

jurecuhalev force-pushed the patch-livereload-6679 branch from 147f599 to c21370f Compare January 17, 2017 20:15

jurecuhalev force-pushed the patch-livereload-6679 branch from 9d1245b to f8eefba Compare January 30, 2017 19:11

stefanpenner approved these changes Jan 30, 2017

View reviewed changes

jurecuhalev force-pushed the patch-livereload-6679 branch from c7353e8 to 4ced5ab Compare February 7, 2017 16:53

Fixes ember-cli#6679 - workaround for tiny-lr not reloading on empty …

503ede1

…files arguments

jurecuhalev force-pushed the patch-livereload-6679 branch from 490077f to 503ede1 Compare February 10, 2017 11:49

homu merged commit 503ede1 into ember-cli:master Feb 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX #6679 - workaround for tiny-lr not reloading on empty files arguments #6690

BUGFIX #6679 - workaround for tiny-lr not reloading on empty files arguments #6690

jurecuhalev commented Jan 16, 2017

stefanpenner commented Jan 16, 2017 •

edited

Loading

stefanpenner Jan 16, 2017

stefanpenner Jan 16, 2017

stefanpenner Jan 16, 2017

jurecuhalev commented Jan 17, 2017

jurecuhalev commented Jan 17, 2017

stefanpenner commented Jan 17, 2017

jurecuhalev commented Jan 17, 2017

stefanpenner commented Jan 17, 2017

jurecuhalev commented Jan 18, 2017

jurecuhalev commented Jan 30, 2017

stefanpenner commented Jan 30, 2017

stefanpenner commented Jan 30, 2017

Turbo87 commented Feb 10, 2017

homu commented Feb 10, 2017

homu commented Feb 10, 2017

homu commented Feb 10, 2017

BUGFIX #6679 - workaround for tiny-lr not reloading on empty files arguments #6690

BUGFIX #6679 - workaround for tiny-lr not reloading on empty files arguments #6690

Conversation

jurecuhalev commented Jan 16, 2017

stefanpenner commented Jan 16, 2017 • edited Loading

stefanpenner Jan 16, 2017

Choose a reason for hiding this comment

stefanpenner Jan 16, 2017

Choose a reason for hiding this comment

stefanpenner Jan 16, 2017

Choose a reason for hiding this comment

jurecuhalev commented Jan 17, 2017

jurecuhalev commented Jan 17, 2017

stefanpenner commented Jan 17, 2017

jurecuhalev commented Jan 17, 2017

stefanpenner commented Jan 17, 2017

jurecuhalev commented Jan 18, 2017

jurecuhalev commented Jan 30, 2017

stefanpenner commented Jan 30, 2017

stefanpenner commented Jan 30, 2017

Turbo87 commented Feb 10, 2017

homu commented Feb 10, 2017

homu commented Feb 10, 2017

homu commented Feb 10, 2017

stefanpenner commented Jan 16, 2017 •

edited

Loading