Skip to content

Commit

Permalink
Update: Rename pauseRequireJS option to fixDependencies (#631)
Browse files Browse the repository at this point in the history 
Rename `pauseRequireJS` option since it's not immediately clear what you'd use it for. We retain support for the older name for backwards compatibility. Also remove unnecessary patching of `require` and `requireJS`.

Fix test in PlainTextViewer that was throwing an unhandled promise rejection.
  • Loading branch information
@tonyjin
tonyjin authored Feb 16, 2018
1 parent 754faa4 commit 77fa2e2
Show file tree
Hide file tree
Showing 8 changed files with 49 additions and 51 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ preview.show(fileId, accessToken, {
| showAnnotations | false | Whether annotations and annotation controls are shown. This option will be overridden by viewer-specific annotation options if they are set. See [Box Annotations](https://github.com/box/box-annotations) for more details |
| showDownload | false | Whether download button is shown |
| useHotkeys | true | Whether hotkeys (keyboard shortcuts) are enabled |
| pauseRequireJS | false | Temporarily disables AMD module loaders (e.g. RequireJS) to allow Preview's third party dependencies to load |
| fixDependencies | false | Temporarily patches AMD to properly load Preview's dependencies. You may need to enable this if your project uses RequireJS |
| disableEventLog | false | Disables client-side `preview` event log. Previewing with this option enabled will not increment access stats (content access is still logged server-side) |
| fileOptions | {} | Mapping of file ID to file-level options. See the file option table below for details |

Expand Down
6 changes: 3 additions & 3 deletions src/lib/Preview.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,9 +814,9 @@ class Preview extends EventEmitter {
// Optional additional query params to append to requests
this.options.queryParams = options.queryParams || {};

// Option to pause requireJS while Preview loads third party dependencies
// RequireJS will be re-enabled on the 'assetsloaded' event fired by Preview
this.options.pauseRequireJS = !!options.pauseRequireJS;
// Option to patch AMD module definitions while Preview loads the third party dependencies it expects in the
// browser global scope. Definitions will be re-enabled on the 'assetsloaded' event
this.options.fixDependencies = !!options.fixDependencies || !!options.pauseRequireJS;

// Option to disable 'preview' event log. Use this if you are using Preview in a way that does not constitute
// a full preview, e.g. a content feed. Enabling this option skips the client-side log to the Events API
Expand Down
18 changes: 9 additions & 9 deletions src/lib/__tests__/Preview-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ describe('lib/Preview', () => {
logoUrl: stubs.logoUrl,
showDownload: true,
showAnnotations: true,
pauseRequireJS: true,
fixDependencies: true,
collection: stubs.collection,
loaders: stubs.loaders
};
Expand Down Expand Up @@ -1056,9 +1056,9 @@ describe('lib/Preview', () => {
expect(preview.options.skipServerUpdate).to.be.true;
});

it('should set whether to pause requireJS when loading dependencies', () => {
it('should set whether to fix dependencies', () => {
preview.parseOptions(preview.previewOptions, stubs.tokens);
expect(preview.options.pauseRequireJS).to.be.true;
expect(preview.options.fixDependencies).to.be.true;
});

it('should add user created loaders before standard loaders', () => {
Expand Down Expand Up @@ -2125,7 +2125,7 @@ describe('lib/Preview', () => {
expect(Timer.reset).to.be.called;
expect(preview.emit).to.not.be.called;
});

it('should reset the timer and escape early if the first load milestone is not hit', () => {
Timer.reset();// Clear out all entries in the Timer
sandbox.stub(Timer, 'reset');
Expand All @@ -2134,14 +2134,14 @@ describe('lib/Preview', () => {
expect(Timer.reset).to.be.called;
expect(preview.emit).to.not.be.called;
});

it('should emit a preview_metric event', (done) => {
preview.on(PREVIEW_METRIC, () => {
done();
});
preview.emitLoadMetrics();
});

it('should emit a preview_metric event with event_name "preview_load"', () => {
const tag = Timer.createTag(preview.file.id, LOAD_METRIC.fullDocumentLoadTime);
Timer.start(tag);
Expand All @@ -2164,7 +2164,7 @@ describe('lib/Preview', () => {
});
preview.emitLoadMetrics();
});

it('should emit a preview_metric event with an object, with all of the proper load properties', () => {
preview.on(PREVIEW_METRIC, (metric) => {
expect(metric[LOAD_METRIC.fileInfoTime]).to.exist;
Expand All @@ -2174,7 +2174,7 @@ describe('lib/Preview', () => {
});
preview.emitLoadMetrics();
});

it('should reset the Timer', () => {
sandbox.stub(Timer, 'reset');
sandbox.stub(preview, 'emit');
Expand All @@ -2183,7 +2183,7 @@ describe('lib/Preview', () => {
expect(preview.emit).to.be.called;

});

it('should default all un-hit milestones, after the first, to 0, and cast float values to ints', () => {
Timer.get(fileInfoTag).elapsed = 1.00001236712394687;
preview.on(PREVIEW_METRIC, (metric) => {
Expand Down
23 changes: 10 additions & 13 deletions src/lib/__tests__/util-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,20 +397,17 @@ describe('lib/util', () => {
assert.ok(head.querySelector('script[src="bar"]') instanceof HTMLScriptElement);
});

it('should clear requireJS until scripts are loaded or fail to load', () => {
window.define = true;
window.require = true;
window.requrejs = true;

return util.loadScripts(['foo', 'bar'], true).catch(() => {
expect(window.define).to.equal(true);
expect(window.require).to.equal(true);
expect(window.requirejs).to.equal(true);
});
it('should disable AMD until scripts are loaded or fail to load', () => {
const func = () => {};
func.amd = ['jquery'];
window.define = func;

const promise = util.loadScripts(['foo', 'bar'], true);
expect(define).to.equal(undefined);

expect(window.define).to.equal(undefined);
expect(window.require).to.equal(undefined);
expect(window.requirejs).to.equal(undefined);
return promise.then(() => {
expect(define).to.equal(func);
});
});
});

Expand Down
31 changes: 14 additions & 17 deletions src/lib/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -475,18 +475,23 @@ export function loadStylesheets(urls) {
*
* @public
* @param {Array} urls - Asset urls
* @param {string} [disableRequireJS] - Should requireJS be temporarily disabled
* @param {string} [disableAMD] - Temporarily disable AMD definitions while external scripts are loading
* @return {Promise} Promise to load scripts
*/
export function loadScripts(urls, disableRequireJS = false) {
export function loadScripts(urls, disableAMD = false) {
const { head } = document;
const promises = [];
const { define, require, requirejs } = window;

if (disableRequireJS) {
window.define = undefined;
window.require = undefined;
window.requirejs = undefined;
// Preview expects third party assets to be loaded into the global scope. However, many of our third party
// assets include a UMD module definition, and a parent application using RequireJS or a similar AMD module loader
// will trigger the AMD check in these UMD definitions and prevent the necessary assets from being loaded in the
// global scope. If `disableAMD` is passed, we get around this by temporarily disabling `define()` until Preview's
// scripts are loaded.

/* eslint-disable no-undef */
const defineRef = typeof define === 'function' ? define : undefined;
if (disableAMD && typeof define === 'function' && define.amd) {
define = undefined;
}

urls.forEach((url) => {
Expand All @@ -504,18 +509,10 @@ export function loadScripts(urls, disableRequireJS = false) {

return Promise.all(promises)
.then(() => {
if (disableRequireJS) {
window.define = define;
window.require = require;
window.requirejs = requirejs;
}
define = defineRef || define;
})
.catch(() => {
if (disableRequireJS) {
window.define = define;
window.require = require;
window.requirejs = requirejs;
}
define = defineRef || define;
});
}

Expand Down
10 changes: 5 additions & 5 deletions src/lib/viewers/BaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,13 +599,12 @@ class BaseViewer extends EventEmitter {
* Loads assets needed for a viewer
*
* @protected
* @param {Array} [js] - js assets
* @param {Array} [css] - css assets
* @param {boolean} [isViewerAsset] is the asset to load third party
* @param {Array} [js] - JS assets
* @param {Array} [css] - CSS assets
* @param {boolean} [isViewerAsset] - Whether we are loading a third party viewer asset
* @return {Promise} Promise to load scripts
*/
loadAssets(js, css, isViewerAsset = true) {
const disableRequireJS = isViewerAsset && !!this.options.pauseRequireJS;
// Create an asset path creator function
const { location } = this.options;
const assetUrlCreator = createAssetUrlCreator(location);
Expand All @@ -614,7 +613,8 @@ class BaseViewer extends EventEmitter {
loadStylesheets((css || []).map(assetUrlCreator));

// Then load the scripts needed for this preview
return loadScripts((js || []).map(assetUrlCreator), disableRequireJS).then(() => {
const disableAMD = isViewerAsset && this.options.fixDependencies;
return loadScripts((js || []).map(assetUrlCreator), disableAMD).then(() => {
if (isViewerAsset) {
this.emit('assetsloaded');
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/viewers/doc/docAssets.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { DOC_STATIC_ASSETS_VERSION } from '../../constants';

const STATIC_URI = `third-party/doc/${DOC_STATIC_ASSETS_VERSION}/`;
export const JS = [`${STATIC_URI}pdf.min.js`, `${STATIC_URI}pdf_viewer.min.js`, `${STATIC_URI}exif.min.js`];
export const JS = [`${STATIC_URI}pdf.js`, `${STATIC_URI}pdf_viewer.min.js`, `${STATIC_URI}exif.min.js`];
export const PRELOAD_JS = [`${STATIC_URI}pdf.worker.min.js`];
export const CSS = [`${STATIC_URI}pdf_viewer.min.css`];
8 changes: 6 additions & 2 deletions src/lib/viewers/text/__tests__/PlainTextViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ describe('lib/viewers/text/PlainTextViewer', () => {

it('should invoke startLoadTimer()', () => {
sandbox.stub(text, 'startLoadTimer');
sandbox.stub(util, 'get').returns(Promise.resolve(''));

text.postLoad();

Expand Down Expand Up @@ -308,21 +309,24 @@ describe('lib/viewers/text/PlainTextViewer', () => {
});

it('should setup the print iframe', () => {
const appendStub = sandbox.stub();

sandbox.stub(util, 'createAssetUrlCreator').returns(sandbox.stub());
sandbox.stub(util, 'openContentInsideIframe').returns({
contentDocument: {
head: {
appendChild: sandbox.stub()
appendChild: appendStub
}
}
});
text.options.location = 'en-US';
sandbox.stub(window, 'setTimeout');

text.preparePrint(['blah']);

expect(util.createAssetUrlCreator).to.be.calledWith(text.options.location);
expect(util.openContentInsideIframe).to.be.calledWith(text.textEl.outerHTML);
expect(text.printframe.contentDocument.head.appendChild).to.be.called.once;
expect(appendStub).to.be.called;
});

it('should enable printing via print popup after a delay', () => {
Expand Down

0 comments on commit 77fa2e2

Please sign in to comment.