Skip to content

Commit

Permalink
Expose option to allow a new sandbox per visit
Browse files Browse the repository at this point in the history
This PR does a few things:

Firstly, it allows the end consumer to decide
if they want a sandbox per visit or to share a single sandbox over all
visits.

Second, it changes the default of `buildSandboxPerVisit` back to
`false` therefore sharing a single sandbox for many `visit` invocations.
  • Loading branch information
rwjblue committed Nov 4, 2019
1 parent 1d0e4c8 commit d5ebdf0
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 10 deletions.
33 changes: 28 additions & 5 deletions src/ember-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ class EmberApp {
destroy() {
// TODO: expose as public api (through the top level) so that we can
// cleanup pre-warmed visits
if (this._applicationInstance) {
this._applicationInstance.destroy();
}
}

/**
Expand Down Expand Up @@ -242,12 +245,25 @@ class EmberApp {
* @param {Object} bootOptions An object containing the boot options that are used by
* by ember to decide whether it needs to do rendering or not.
* @param {Object} result
* @param {Boolean} buildSandboxPerVisit if true, a new sandbox will
* **always** be created, otherwise one
* is created for the first request
* only
* @return {Promise<instance>} instance
*/
async visitRoute(path, fastbootInfo, bootOptions, result) {
let app = await this.buildApp();
result.applicationInstance = app;

async _visit(path, fastbootInfo, bootOptions, result, buildSandboxPerVisit) {
let shouldBuildApp = buildSandboxPerVisit || this._applicationInstance === undefined;

let app = shouldBuildApp ? await this.buildApp() : this._applicationInstance;

// entangle the specific application instance to the result, so it can be
// destroyed when result._destroy() is called (after the visit is
// completed)
if (buildSandboxPerVisit) {
result.applicationInstance = app;
} else {
this._applicationInstance = app;
}
await app.boot();

let instance = await app.buildInstance();
Expand Down Expand Up @@ -278,6 +294,7 @@ class EmberApp {
* @param {Boolean} [options.shouldRender] whether the app should do rendering or not. If set to false, it puts the app in routing-only.
* @param {Boolean} [options.disableShoebox] whether we should send the API data in the shoebox. If set to false, it will not send the API data used for rendering the app on server side in the index.html.
* @param {Integer} [options.destroyAppInstanceInMs] whether to destroy the instance in the given number of ms. This is a failure mechanism to not wedge the Node process (See: https://github.com/ember-fastboot/fastboot/issues/90)
* @param {Boolean} [options.buildSandboxPerVisit] whether to create a new sandbox context per-visit, or reuse the existing sandbox
* @param {ClientRequest}
* @param {ClientResponse}
* @returns {Promise<Result>} result
Expand Down Expand Up @@ -314,7 +331,13 @@ class EmberApp {
}

try {
await this.visitRoute(path, fastbootInfo, bootOptions, result);
await this._visit(
path,
fastbootInfo,
bootOptions,
result,
options.buildSandboxPerVisit === true
);

if (!disableShoebox) {
// if shoebox is not disabled, then create the shoebox and send API data
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class FastBoot {
* @param {Boolean} [options.shouldRender] whether the app should do rendering or not. If set to false, it puts the app in routing-only.
* @param {Boolean} [options.disableShoebox] whether we should send the API data in the shoebox. If set to false, it will not send the API data used for rendering the app on server side in the index.html.
* @param {Integer} [options.destroyAppInstanceInMs] whether to destroy the instance in the given number of ms. This is a failure mechanism to not wedge the Node process (See: https://github.com/ember-fastboot/fastboot/issues/90)
* @param {Boolean} [options.buildSandboxPerVisit] whether to create a new sandbox context per-visit (slows down each visit, but guarantees no prototype leakages can occur), or reuse the existing sandbox (faster per-request, but each request shares the same set of prototypes)
* @returns {Promise<Result>} result
*/
async visit(path, options = {}) {
Expand Down
13 changes: 8 additions & 5 deletions test/fastboot-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -401,45 +401,48 @@ describe('FastBoot', function() {
});
});

it('in app prototype mutations do not leak across visits', async function() {
it('in app prototype mutations do not leak across visits with buildSandboxPerVisit=true', async function() {
this.timeout(3000);

var fastboot = new FastBoot({
distPath: fixture('app-with-prototype-mutations'),
});

let result = await fastboot.visit('/');
let result = await fastboot.visit('/', { buildSandboxPerVisit: true });
let html = await result.html();

expect(html).to.match(/Items: 1/);

result = await fastboot.visit('/');
result = await fastboot.visit('/', { buildSandboxPerVisit: true });
html = await result.html();

expect(html).to.match(/Items: 1/);

result = await fastboot.visit('/');
result = await fastboot.visit('/', { buildSandboxPerVisit: true });
html = await result.html();

expect(html).to.match(/Items: 1/);
});

it('errors can be properly attributed', async function() {
it('errors can be properly attributed with buildSandboxPerVisit=true', async function() {
this.timeout(3000);

var fastboot = new FastBoot({
distPath: fixture('onerror-per-visit'),
});

let first = fastboot.visit('/slow/100/reject', {
buildSandboxPerVisit: true,
request: { url: '/slow/100/reject', headers: {} },
});

let second = fastboot.visit('/slow/50/resolve', {
buildSandboxPerVisit: true,
request: { url: '/slow/50/resolve', headers: {} },
});

let third = fastboot.visit('/slow/25/resolve', {
buildSandboxPerVisit: true,
request: { url: '/slow/25/resolve', headers: {} },
});

Expand Down

0 comments on commit d5ebdf0

Please sign in to comment.