Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Removing autobind from Preview.js #500

Merged
merged 2 commits into from
Nov 21, 2017
+52 −55
Merged

Chore: Removing autobind from Preview.js #500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e640822
Chore: Removing autobind from Preview.js
Nov 21, 2017
f65eb19
Merge branch 'master' into remove-preview-autobind
tonyjin Nov 21, 2017
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
38 changes: 21 additions & 17 deletions src/lib/Preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/* eslint-disable import/first */
import './polyfill';
import autobind from 'autobind-decorator';
import EventEmitter from 'events';
import throttle from 'lodash.throttle';
import cloneDeep from 'lodash.clonedeep';
Expand Down Expand Up @@ -54,7 +53,6 @@ const LOG_RETRY_COUNT = 3; // number of times to retry logging preview event
// and not when preview is instantiated, which is too late.
const PREVIEW_LOCATION = findScriptLocation(PREVIEW_SCRIPT_NAME, document.currentScript);

@autobind
class Preview extends EventEmitter {
/** @property {boolean} - Whether preview is open */
open = false;
Expand Down Expand Up @@ -138,6 +136,18 @@ class Preview extends EventEmitter {
this.cache = new Cache();
this.ui = new PreviewUI();
this.browserInfo = Browser.getBrowserInfo();

// Bind context for callbacks
this.print = this.print.bind(this);
this.handleTokenResponse = this.handleTokenResponse.bind(this);
this.handleFileInfoResponse = this.handleFileInfoResponse.bind(this);
this.handleFetchError = this.handleFetchError.bind(this);
this.handleViewerEvents = this.handleViewerEvents.bind(this);
this.triggerError = this.triggerError.bind(this);
this.throttledMousemoveHandler = this.getGlobalMousemoveHandler().bind(this);
this.navigateLeft = this.navigateLeft.bind(this);
this.navigateRight = this.navigateRight.bind(this);
this.keydownHandler = this.keydownHandler.bind(this);
}

/**
Expand Down Expand Up @@ -582,8 +592,8 @@ class Preview extends EventEmitter {

// Fetch access tokens before proceeding
getTokens(this.file.id, this.previewOptions.token)
.then(this.loadPreviewWithTokens)
.catch(this.triggerFetchError);
.then(this.handleTokenResponse)
.catch(this.handleFetchError);
}

/**
Expand All @@ -593,7 +603,7 @@ class Preview extends EventEmitter {
* @param {Object} tokenMap - Map of file ID to access token
* @return {void}
*/
loadPreviewWithTokens(tokenMap) {
handleTokenResponse(tokenMap) {
// If this is a retry, short-circuit and load from server
if (this.retryCount > 0) {
this.loadFromServer();
Expand All @@ -609,7 +619,7 @@ class Preview extends EventEmitter {
this.keydownHandler,
this.navigateLeft,
this.navigateRight,
this.getGlobalMousemoveHandler()
this.throttledMousemoveHandler
);

// Setup loading UI and progress bar
Expand Down Expand Up @@ -747,8 +757,8 @@ class Preview extends EventEmitter {

const fileInfoUrl = appendQueryParams(getURL(this.file.id, apiHost), queryParams);
get(fileInfoUrl, this.getRequestHeaders())
.then(this.handleLoadResponse)
.catch(this.triggerFetchError);
.then(this.handleFileInfoResponse)
.catch(this.handleFetchError);
}

/**
Expand All @@ -758,7 +768,7 @@ class Preview extends EventEmitter {
* @param {Object} file - File object
* @return {void}
*/
handleLoadResponse(file) {
handleFileInfoResponse(file) {
// If preview is closed or response comes back for an incorrect file, don't do anything
if (!this.open || (this.file && this.file.id !== file.id)) {
return;
Expand Down Expand Up @@ -1058,7 +1068,7 @@ class Preview extends EventEmitter {
* @param {Object} err Error object
* @return {void}
*/
triggerFetchError(err) {
handleFetchError(err) {
// If preview is closed don't do anything
if (!this.open) {
return;
Expand Down Expand Up @@ -1227,11 +1237,7 @@ class Preview extends EventEmitter {
* @return {Function} Throttled mousemove handler
*/
getGlobalMousemoveHandler() {
if (this.throttledMousemoveHandler) {
return this.throttledMousemoveHandler;
}

this.throttledMousemoveHandler = throttle(
return throttle(
() => {
clearTimeout(this.timeoutHandler);

Expand Down Expand Up @@ -1262,8 +1268,6 @@ class Preview extends EventEmitter {
MOUSEMOVE_THROTTLE - 500,
true
);

return this.throttledMousemoveHandler;
}

/**
Expand Down
69 changes: 31 additions & 38 deletions src/lib/__tests__/Preview-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ describe('lib/Preview', () => {
});

stubs.getTokens = sandbox.stub(tokens, 'default').returns(stubs.promise);
stubs.loadPreviewWithTokens = sandbox.stub(preview, 'loadPreviewWithTokens');
stubs.handleTokenResponse = sandbox.stub(preview, 'handleTokenResponse');
stubs.get = sandbox.stub(preview.cache, 'get');
stubs.destroy = sandbox.stub(preview, 'destroy');
});
Expand Down Expand Up @@ -757,7 +757,7 @@ describe('lib/Preview', () => {
preview.load({ id: '123' });
return stubs.promise.then(() => {
expect(stubs.getTokens).to.be.calledWith('123', 'token');
expect(stubs.loadPreviewWithTokens).to.be.called;
expect(stubs.handleTokenResponse).to.be.called;
});
});

Expand All @@ -767,12 +767,12 @@ describe('lib/Preview', () => {
preview.load('0');
return stubs.promise.then(() => {
expect(stubs.getTokens).to.be.calledWith('0', 'token');
expect(stubs.loadPreviewWithTokens).to.be.called;
expect(stubs.handleTokenResponse).to.be.called;
});
});
});

describe('loadPreviewWithTokens()', () => {
describe('handleTokenResponse()', () => {
beforeEach(() => {
stubs.loadFromServer = sandbox.stub(preview, 'loadFromServer');
stubs.parseOptions = sandbox.stub(preview, 'parseOptions');
Expand All @@ -789,34 +789,34 @@ describe('lib/Preview', () => {
it('should short circuit and load from server if it is a retry', () => {
preview.retryCount = 1;

preview.loadPreviewWithTokens({});
preview.handleTokenResponse({});
expect(stubs.loadFromServer).to.be.called;
expect(stubs.parseOptions).to.not.be.called;
});

it('should parse the preview options', () => {
preview.retryCount = 0;

preview.loadPreviewWithTokens({});
preview.handleTokenResponse({});
expect(stubs.parseOptions).to.be.called;
});

it('should setup the container and show the loading indicator and progress bar', () => {
preview.loadPreviewWithTokens({});
preview.handleTokenResponse({});
expect(stubs.setup).to.be.called;
expect(stubs.showLoadingIndicator).to.be.called;
expect(stubs.startProgressBar).to.be.called;
});

it('should show navigation', () => {
preview.loadPreviewWithTokens({});
preview.handleTokenResponse({});
expect(stubs.showNavigation).to.be.called;
});

it('should load from cache if the file is valid', () => {
stubs.checkFileValid.returns(true);

preview.loadPreviewWithTokens({});
preview.handleTokenResponse({});
expect(stubs.cacheFile).to.be.calledWith(preview.cache, preview.file);
expect(stubs.loadFromCache).to.be.called;
expect(stubs.loadFromServer).to.not.be.called;
Expand All @@ -825,7 +825,7 @@ describe('lib/Preview', () => {
it('should load from the server on a cache miss', () => {
stubs.checkFileValid.returns(false);

preview.loadPreviewWithTokens({});
preview.handleTokenResponse({});
expect(stubs.loadFromCache).to.not.be.called;
expect(stubs.loadFromServer).to.be.called;
});
Expand Down Expand Up @@ -1008,8 +1008,8 @@ describe('lib/Preview', () => {
beforeEach(() => {
stubs.promise = Promise.resolve('file');
stubs.get = sandbox.stub(util, 'get').returns(stubs.promise);
stubs.handleLoadResponse = sandbox.stub(preview, 'handleLoadResponse');
stubs.triggerFetchError = sandbox.stub(preview, 'triggerFetchError');
stubs.handleFileInfoResponse = sandbox.stub(preview, 'handleFileInfoResponse');
stubs.handleFetchError = sandbox.stub(preview, 'handleFetchError');
stubs.getURL = sandbox.stub(file, 'getURL').returns('/get_url');
preview.file = {
id: 0
Expand All @@ -1021,13 +1021,13 @@ describe('lib/Preview', () => {
expect(stubs.get).to.be.called;
expect(stubs.getURL).to.be.called;
return stubs.promise.then(() => {
expect(stubs.handleLoadResponse).to.be.called;
expect(stubs.triggerFetchError).to.not.be.called;
expect(stubs.handleFileInfoResponse).to.be.called;
expect(stubs.handleFetchError).to.not.be.called;
});
});
});

describe('handleLoadResponse()', () => {
describe('handleFileInfoResponse()', () => {
beforeEach(() => {
preview.logger = {
setFile: sandbox.stub(),
Expand Down Expand Up @@ -1056,7 +1056,7 @@ describe('lib/Preview', () => {

it('should do nothing if the preview is closed', () => {
preview.open = false;
preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(stubs.set).to.not.be.called;
});

Expand All @@ -1067,7 +1067,7 @@ describe('lib/Preview', () => {
};
stubs.file.id = 1;

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(stubs.set).to.not.be.called;
});

Expand All @@ -1077,7 +1077,7 @@ describe('lib/Preview', () => {
id: 0
};

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(preview.file).to.equal(stubs.file);
expect(preview.logger.setFile).to.be.called;
});
Expand All @@ -1096,7 +1096,7 @@ describe('lib/Preview', () => {

stubs.file.file_version.sha1 = 0;

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(stubs.get).to.be.calledWith(stubs.file.id);
expect(stubs.set).to.be.calledWith(stubs.file.id);
expect(stubs.loadViewer).to.not.be.called;
Expand All @@ -1117,7 +1117,7 @@ describe('lib/Preview', () => {

stubs.file.file_version.sha1 = 0;

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(stubs.set).to.be.not.be.called;
});

Expand All @@ -1129,7 +1129,7 @@ describe('lib/Preview', () => {

stubs.get.returns(false);

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(preview.logger.setCacheStale).to.be.called;
expect(stubs.loadViewer).to.be.called;
});
Expand All @@ -1142,7 +1142,7 @@ describe('lib/Preview', () => {

stubs.checkFileValid.returns(false);

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(preview.logger.setCacheStale).to.be.called;
expect(stubs.loadViewer).to.be.called;
});
Expand All @@ -1161,7 +1161,7 @@ describe('lib/Preview', () => {

stubs.file.file_version.sha1 = 2;

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(preview.logger.setCacheStale).to.be.called;
expect(stubs.loadViewer).to.be.called;
});
Expand All @@ -1181,7 +1181,7 @@ describe('lib/Preview', () => {

stubs.file.file_version.sha1 = 2;

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(preview.logger.setCacheStale).to.be.called;
expect(stubs.loadViewer).to.be.called;
});
Expand All @@ -1194,7 +1194,7 @@ describe('lib/Preview', () => {

stubs.get.throws(new Error());

preview.handleLoadResponse(stubs.file);
preview.handleFileInfoResponse(stubs.file);
expect(stubs.triggerError).to.be.called;
});
});
Expand Down Expand Up @@ -1641,7 +1641,7 @@ describe('lib/Preview', () => {
});
});

describe('triggerFetchError()', () => {
describe('handleFetchError()', () => {
beforeEach(() => {
stubs.unset = sandbox.stub(preview.cache, 'unset');
stubs.triggerError = sandbox.stub(preview, 'triggerError');
Expand All @@ -1659,7 +1659,7 @@ describe('lib/Preview', () => {
};
preview.open = false;

preview.triggerFetchError(stubs.error);
preview.handleFetchError(stubs.error);
expect(stubs.unset).to.not.be.called;
});

Expand All @@ -1669,7 +1669,7 @@ describe('lib/Preview', () => {
};
preview.open = true;

preview.triggerFetchError(stubs.error);
preview.handleFetchError(stubs.error);
expect(stubs.unset).to.be.called;
});

Expand All @@ -1680,7 +1680,7 @@ describe('lib/Preview', () => {
preview.open = true;
preview.retryCount = 6;

preview.triggerFetchError(stubs.error);
preview.handleFetchError(stubs.error);
expect(stubs.triggerError).to.be.called;
});

Expand All @@ -1692,7 +1692,7 @@ describe('lib/Preview', () => {
preview.retryCount = 6;
stubs.error.response.status = 429;

preview.triggerFetchError(stubs.error);
preview.handleFetchError(stubs.error);
try {
expect(stubs.triggerError).to.be.calledWith(new Error(__('error_rate_limit')));
} catch (e) {
Expand All @@ -1709,7 +1709,7 @@ describe('lib/Preview', () => {
preview.retryCount = 1;
preview.file.id = 1;

preview.triggerFetchError(stubs.error);
preview.handleFetchError(stubs.error);
expect(stubs.triggerError).to.not.be.called;

clock.tick(RETRY_TIMEOUT + 1);
Expand Down Expand Up @@ -1907,13 +1907,6 @@ describe('lib/Preview', () => {
});

describe('getGlobalMousemoveHandler()', () => {
it('should return the throttled mouse move handler if it already exists', () => {
preview.throttledMousemoveHandler = true;

const handler = preview.getGlobalMousemoveHandler();
expect(handler).to.be.true;
});

it('should clear the timeout handler and do nothing if the container doesn\'t exist', () => {
preview.container = false;

Expand Down