-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 7 commits
9c31dbe
c48ae07
c0cfd57
d6244ec
b8943e8
b9b8617
e0be3b8
8ca1c33
2162dde
5044600
6d0cf8b
71e9bfc
4fc8706
dafaee9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
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 \//); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); | ||
}); |
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'); |
There was a problem hiding this comment.
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 ofassertBufferMatch
? It seems to be supported...There was a problem hiding this comment.
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
There was a problem hiding this comment.
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