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.

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

[optimize] inject publicPath at request time #14007

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@
"less": "2.7.1",
"less-loader": "2.2.3",
"lodash": "3.10.1",
"lru-cache": "4.1.1",
"markdown-it": "8.3.2",
"minimatch": "2.0.10",
"mkdirp": "0.5.1",
Expand Down Expand Up @@ -188,6 +189,7 @@
"rimraf": "2.4.3",
"rison-node": "1.0.0",
"rjs-repack-loader": "1.0.6",
"rxjs": "5.4.3",
"script-loader": "0.6.1",
"semver": "5.1.0",
"style-loader": "0.12.3",
Expand Down
5 changes: 3 additions & 2 deletions src/optimize/base_optimizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ import { defaults, transform } from 'lodash';
import { fromRoot } from '../utils';
import babelOptions from './babel/options';
import pkg from '../../package.json';

import { PUBLIC_PATH_PLACEHOLDER } from './public_path_placeholder';
import { setLoaderQueryParam, makeLoaderString } from './loaders';

const babelExclude = [/[\/\\](webpackShims|node_modules|bower_components)[\/\\]/];

export default class BaseOptimizer {
constructor(opts) {
this.env = opts.env;
this.urlBasePath = opts.urlBasePath;
this.bundles = opts.bundles;
this.profile = opts.profile || false;

Expand Down Expand Up @@ -101,7 +102,7 @@ export default class BaseOptimizer {
path: this.env.workingDir,
filename: '[name].bundle.js',
sourceMapFilename: '[file].map',
publicPath: `${this.urlBasePath || ''}/bundles/`,
publicPath: PUBLIC_PATH_PLACEHOLDER,
devtoolModuleFilenameTemplate: '[absolute-resource-path]'
},

Expand Down
314 changes: 314 additions & 0 deletions src/optimize/bundles_route/__tests__/bundles_route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,314 @@
import { resolve } from 'path';
import { readFileSync } from 'fs';
import crypto from 'crypto';

import Chance from 'chance';
import expect from 'expect.js';
import Hapi from 'hapi';
import Inert from 'inert';
import sinon from 'sinon';

import { createBundlesRoute } from '../bundles_route';
import { PUBLIC_PATH_PLACEHOLDER } from '../../public_path_placeholder';

const chance = new Chance();
const outputFixture = resolve(__dirname, './fixtures/output');

function replaceAll(source, replace, replaceWith) {
return source.split(replace).join(replaceWith);
}

function assertBufferMatch(a, b) {
if (a.length !== b.length || a.indexOf(b) !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

question: why not just expect(a).to.eql(b) instead of assertBufferMatch? It seems to be supported...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, I guess I didn't check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense I suppose, they're basically just arrays

throw new Error('Expected buffers to match');
}
}

describe('optimizer/bundle route', () => {
const sandbox = sinon.sandbox.create();

function createServer(options = {}) {
const {
bundlesPath = outputFixture,
basePublicPath = ''
} = options;

const server = new Hapi.Server();
server.connection({ port: 0 });
server.register([Inert]);

server.route(createBundlesRoute({
bundlesPath,
basePublicPath,
}));

return server;
}

afterEach(() => sandbox.restore());

describe('validation', () => {
it('validates that bundlesPath is an absolute path', () => {
expect(() => {
createBundlesRoute({
bundlesPath: null,
basePublicPath: ''
});
}).to.throwError(/absolute path/);
expect(() => {
createBundlesRoute({
bundlesPath: './relative',
basePublicPath: ''
});
}).to.throwError(/absolute path/);
expect(() => {
createBundlesRoute({
bundlesPath: 1234,
basePublicPath: ''
});
}).to.throwError(/absolute path/);
expect(() => {
createBundlesRoute({
bundlesPath: '/absolute/path',
basePublicPath: ''
});
}).to.not.throwError();
});
it('validates that basePublicPath is valid', () => {
expect(() => {
createBundlesRoute({
bundlesPath: '/bundles',
basePublicPath: 123
});
}).to.throwError(/string/);
expect(() => {
createBundlesRoute({
bundlesPath: '/bundles',
basePublicPath: {}
});
}).to.throwError(/string/);
expect(() => {
createBundlesRoute({
bundlesPath: '/bundles',
basePublicPath: '/a/'
});
}).to.throwError(/start and not end with a \//);
expect(() => {
createBundlesRoute({
bundlesPath: '/bundles',
basePublicPath: 'a/'
});
}).to.throwError(/start and not end with a \//);
Copy link
Member

Choose a reason for hiding this comment

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

nit: could you please add case for the happy path too, as in the test above?

});
});

describe('image', () => {
it('responds with exact file data', async () => {
const server = createServer();
const response = await server.inject({
url: '/bundles/image.png'
});

expect(response.statusCode).to.be(200);
const image = readFileSync(resolve(outputFixture, 'image.png'));
expect(response.headers).to.have.property('content-length', image.length);
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: our code is responsible for setting correct content-type now, maybe it makes sense to verify it here too.

assertBufferMatch(image, response.rawPayload);
});
});

describe('js file without placeholder', () => {
it('responds with no content-length and exact file data', async () => {
const server = createServer();
const response = await server.inject({
url: '/bundles/no_placeholder.js'
});

expect(response.statusCode).to.be(200);
expect(response.headers).to.not.have.property('content-length');
assertBufferMatch(
readFileSync(resolve(outputFixture, 'no_placeholder.js')),
response.rawPayload
);
});
});

describe('js file with placeholder', () => {
it('responds with no content-length and modified file data', async () => {
const basePublicPath = `/${chance.word()}`;
const server = createServer({ basePublicPath });

const response = await server.inject({
url: '/bundles/with_placeholder.js'
});

expect(response.statusCode).to.be(200);
const source = readFileSync(resolve(outputFixture, 'with_placeholder.js'), 'utf8');
expect(response.headers).to.not.have.property('content-length');
expect(response.result.indexOf(source)).to.be(-1);
expect(response.result).to.be(replaceAll(
source,
PUBLIC_PATH_PLACEHOLDER,
`${basePublicPath}/bundles/`
));
});
});

describe('css file without placeholder', () => {
it('responds with no content-length and exact file data', async () => {
const server = createServer();
const response = await server.inject({
url: '/bundles/no_placeholder.css'
});

expect(response.statusCode).to.be(200);
expect(response.headers).to.not.have.property('content-length');
assertBufferMatch(
readFileSync(resolve(outputFixture, 'no_placeholder.css')),
response.rawPayload
);
});
});

describe('css file with placeholder', () => {
it('responds with no content-length and modified file data', async () => {
const basePublicPath = `/${chance.word()}`;
const server = createServer({ basePublicPath });

const response = await server.inject({
url: '/bundles/with_placeholder.css'
});

expect(response.statusCode).to.be(200);
const source = readFileSync(resolve(outputFixture, 'with_placeholder.css'), 'utf8');
expect(response.headers).to.not.have.property('content-length');
expect(response.result.indexOf(source)).to.be(-1);
expect(response.result).to.be(replaceAll(
source,
PUBLIC_PATH_PLACEHOLDER,
`${basePublicPath}/bundles/`
));
});
});

describe('js file outside bundlesPath', () => {
it('responds with a 403', async () => {
const server = createServer();

const response = await server.inject({
url: '/bundles/../outside_output.js'
});

expect(response.statusCode).to.be(403);
expect(response.result).to.eql({
error: 'Forbidden',
message: 'Forbidden',
statusCode: 403
});
});
});

describe('missing js file', () => {
it('responds with 404', async () => {
const server = createServer();

const response = await server.inject({
url: '/bundles/non_existant.js'
});

expect(response.statusCode).to.be(404);
expect(response.result).to.eql({
error: 'Not Found',
message: 'Not Found',
statusCode: 404
});
});
});

describe('missing bundlesPath', () => {
it('responds with 404', async () => {
const server = createServer({
bundlesPath: resolve(__dirname, 'fixtures/not_really_output')
});

const response = await server.inject({
url: '/bundles/with_placeholder.js'
});

expect(response.statusCode).to.be(404);
expect(response.result).to.eql({
error: 'Not Found',
message: 'Not Found',
statusCode: 404
});
});
});

describe('etag', () => {
it('only calculates hash of file on first request', async () => {
const createHash = sandbox.spy(crypto, 'createHash');

const server = createServer();

sinon.assert.notCalled(createHash);
const resp1 = await server.inject({
url: '/bundles/no_placeholder.js'
});

sinon.assert.calledOnce(createHash);
createHash.reset();
expect(resp1.statusCode).to.be(200);

const resp2 = await server.inject({
url: '/bundles/no_placeholder.js'
});

sinon.assert.notCalled(createHash);
expect(resp2.statusCode).to.be(200);
});

it('is unique per basePublicPath although content is the same', async () => {
const basePublicPath1 = `/${chance.word()}`;
const basePublicPath2 = `/${chance.word()}`;

const [resp1, resp2] = await Promise.all([
createServer({ basePublicPath: basePublicPath1 }).inject({
url: '/bundles/no_placeholder.js'
}),
createServer({ basePublicPath: basePublicPath2 }).inject({
url: '/bundles/no_placeholder.js'
}),
]);

expect(resp1.statusCode).to.be(200);
expect(resp2.statusCode).to.be(200);

assertBufferMatch(resp1.rawPayload, resp2.rawPayload);

expect(resp1.headers.etag).to.be.a('string');
expect(resp2.headers.etag).to.be.a('string');
expect(resp1.headers.etag).to.not.eql(resp2.headers.etag);
});
});

describe('cache control', () => {
it('responds with 304 when etag and last modified are sent back', async () => {
const server = createServer();
const resp = await server.inject({
url: '/bundles/with_placeholder.js'
});

expect(resp.statusCode).to.be(200);

const resp2 = await server.inject({
url: '/bundles/with_placeholder.js',
headers: {
'if-modified-since': resp.headers['last-modified'],
'if-none-match': resp.headers.etag
}
});

expect(resp2.statusCode).to.be(304);
expect(resp2.result).to.have.length(0);
});
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body {
background-color: goldenrod;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('chunk2');
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body {
background-image: url(__REPLACE_WITH_PUBLIC_PATH__/image.png);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('__REPLACE_WITH_PUBLIC_PATH__');
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('outside output');
Loading