Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions src/extensions/default/CSSShapesEditor/LiveEditorLocalDriver.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ define(function (require, exports, module) {
}

/**
* @private
* Inject remote live editor driver and any specified editor providers.
* The remote live editor driver mirrors most of the local live editor driver API
* to provide an interface to the in-browser live editor.
Expand All @@ -104,16 +103,16 @@ define(function (require, exports, module) {
}

/**
* @private
* Send instructions to remove the live editor from the page in LivePreview.
* @return {$.Promise}
*/
function remove() {
if (_hasEditor === false) {
var deferred = $.Deferred();
return deferred.reject();
return deferred.reject().promise();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to return return deferred.reject().promise() on line 73 inside _call() function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already returns .reject() on line 73

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I updated it to reject().promise() after I realized that I forgot to append the promise(). So it should have .promise().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And actually this is from @TomMalbran in this comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 73 it returns .reject() so it may be caught by _whenRemoteCallFailed() downstream.

In that scenario (Inspector not connected), it will attempt reconnect a few times, then finally give up and remove the editor. Returning .reject().promise() instead won't bypass this workflow?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this line it is just a minor nit, so that the returned objects are always the same and correspond to the JSDocs.

The idea of returning a Promise is so that the function that uses it can't call resolve or reject over the deferred object. But since you are already returning a rejected deferred, the state can't be changed so returning a promise won't change that. You can still use then, done, fail, an all those methods on a promise.

}

_cache.model = undefined; // do not move in _reset(), otherwhise the _reconnect() scenario misses the cache and fails
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still one typo: otherwhise

_reset();
var expr = _namespace + ".remove()";
return _call(expr);
Expand All @@ -138,6 +137,10 @@ define(function (require, exports, module) {
hasChanged = false,
key;

if (!data) {
remove();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oslego We should also return if (!data). Otherwise, we need to check data again before accessing it on line 153 data.forceUpdate.


// sync the local model snapshot with the remote model
_.forEach(data, function (value, key) {
if (!_model[key] || !_.isEqual(_model[key], value)) {
Expand All @@ -155,18 +158,26 @@ define(function (require, exports, module) {
/**
* @private
* Handle failed promises for eval() calls to the inspected page.
* Promises can fail if the user manually refreshes the page or navigates
* Promises fail if the user manually refreshes the page or navigates
* because the injected editor files will be lost.
* If this is the case, attempt to reconnect.
*
* Promises also fail because of errors thrown in the remote page.
* If this is the case, remove the editor.
*
* @param {$.Promise=} result promise result
* @param {$.Deferred=} result
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you are no longer using this parameter

*/
function _whenRemoteCallFailed(result) {
if (result) {
return _reconnect();
} else {
_cache.model = undefined;
return remove();
}
// check if the remote editor namespace is still defined on the page
_call(_namespace)
.then(function (response) {
if (response.type === "undefined") {
_reconnect();
} else {
remove();
}
})
.fail(remove);
}

/**
Expand Down
11 changes: 0 additions & 11 deletions src/extensions/default/CSSShapesEditor/unittests.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,17 +466,6 @@ define(function (require, exports, module) {

expect(LiveEditorLocalDriver.remove).toHaveBeenCalled();
});

it("should call remove() when LivePreview is turned off", function () {
var deferred = $.Deferred();
spyOn(LiveEditorLocalDriver, "remove").andReturn(deferred.promise());

main._setup();
main._teardown();

expect(LiveEditorLocalDriver.remove).toHaveBeenCalled();
});

});

describe("Live Preview Workflow", function () {
Expand Down