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

Jump to bottom

Invalidate cache for link[preload] in DEV #6183

Merged
merged 5 commits into from
Feb 3, 2019
+71 −17
Merged

Invalidate cache for link[preload] in DEV #6183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d230b7
Invalidate cache for link[preload] in DEV
giuseppeg Jan 31, 2019
746670d
Add timeout to scripts src too
giuseppeg Jan 31, 2019
e8b53a0
Do not define timestamp in prod, add prod testcase
giuseppeg Feb 1, 2019
12e7e05
Remove duplication
giuseppeg Feb 2, 2019
5612ffb
add query string safeguard test
giuseppeg Feb 2, 2019
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
51 changes: 34 additions & 17 deletions packages/next/pages/_document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ const Fragment = React.Fragment || function Fragment ({ children }) {

export default class Document extends Component {
static childContextTypes = {
_documentProps: PropTypes.any
_documentProps: PropTypes.any,
_devOnlyInvalidateCacheQueryString: PropTypes.string,
Copy link
Member

@timneutkens timneutkens Feb 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should this use React 16.3 context instead?

Copy link
Member

Choose a reason for hiding this comment

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

The documentProps one is kinda tricky but your new one might make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will think of some way to refactor this. With the current implementation I think that we would need to add the provider in next-server as the Document render method could be overridden.

After all probably your idea of adding the timestamp in next-server was better - we could define all the files to render there (also the hardcoded ones) and have this component just render them.

}

static getInitialProps ({ renderPage }) {
Expand All @@ -20,7 +21,13 @@ export default class Document extends Component {
}

getChildContext () {
return { _documentProps: this.props }
return {
_documentProps: this.props,
// In dev we invalidate the cache by appending a timestamp to the resource URL.
// This is a workaround to fix https://github.com/zeit/next.js/issues/5860
// TODO: remove this workaround when https://bugs.webkit.org/show_bug.cgi?id=187726 is fixed.
_devOnlyInvalidateCacheQueryString: process.env.NODE_ENV !== 'production' ? '?ts=' + Date.now() : ''
}
}

render () {
Expand All @@ -36,7 +43,8 @@ export default class Document extends Component {

export class Head extends Component {
static contextTypes = {
_documentProps: PropTypes.any
_documentProps: PropTypes.any,
_devOnlyInvalidateCacheQueryString: PropTypes.string,
}

static propTypes = {
Expand Down Expand Up @@ -68,11 +76,13 @@ export class Head extends Component {

getPreloadDynamicChunks () {
const { dynamicImports, assetPrefix } = this.context._documentProps
const { _devOnlyInvalidateCacheQueryString } = this.context

return dynamicImports.map((bundle) => {
return <link
rel='preload'
key={bundle.file}
href={`${assetPrefix}/_next/${bundle.file}`}
href={`${assetPrefix}/_next/${bundle.file}${_devOnlyInvalidateCacheQueryString}`}
as='script'
nonce={this.props.nonce}
crossOrigin={this.props.crossOrigin || process.crossOrigin}
Expand All @@ -82,9 +92,10 @@ export class Head extends Component {

getPreloadMainLinks () {
const { assetPrefix, files } = this.context._documentProps
if(!files || files.length === 0) {
if (!files || files.length === 0) {
return null
}
const { _devOnlyInvalidateCacheQueryString } = this.context

return files.map((file) => {
// Only render .js files here
Expand All @@ -96,7 +107,7 @@ export class Head extends Component {
key={file}
nonce={this.props.nonce}
rel='preload'
href={`${assetPrefix}/_next/${file}`}
href={`${assetPrefix}/_next/${file}${_devOnlyInvalidateCacheQueryString}`}
as='script'
crossOrigin={this.props.crossOrigin || process.crossOrigin}
/>
Expand All @@ -105,6 +116,7 @@ export class Head extends Component {

render () {
const { head, styles, assetPrefix, __NEXT_DATA__ } = this.context._documentProps
const { _devOnlyInvalidateCacheQueryString } = this.context
const { page, buildId } = __NEXT_DATA__
const pagePathname = getPagePathname(page)

Expand All @@ -119,12 +131,11 @@ export class Head extends Component {
})
if (this.props.crossOrigin) console.warn('Warning: `Head` attribute `crossOrigin` is deprecated. https://err.sh/next.js/doc-crossorigin-deprecated')
}

return <head {...this.props}>
{children}
{head}
{page !== '/_error' && <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages${pagePathname}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />}
<link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_app.js`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
{page !== '/_error' && <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages${pagePathname}${_devOnlyInvalidateCacheQueryString}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />}
<link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_app.js${_devOnlyInvalidateCacheQueryString}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
{this.getPreloadDynamicChunks()}
{this.getPreloadMainLinks()}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

out of curiosity why the main links are after the dynamic ones?

Copy link
Member

Choose a reason for hiding this comment

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

The dynamic ones here are actually needed for hydration anyway, so the other isn't important as we'd wait for these to load before doing anything else anyway currently

Copy link
Member

Choose a reason for hiding this comment

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

Note these are only the dynamic components that have been rendered not all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sure, is that the case for client side rendering too though?

Copy link
Member

Choose a reason for hiding this comment

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

@giuseppeg those are handled by webpack's dynamic loading mechanism.

{this.getCssLinks()}
Expand All @@ -135,7 +146,8 @@ export class Head extends Component {

export class Main extends Component {
static contextTypes = {
_documentProps: PropTypes.any
_documentProps: PropTypes.any,
_devOnlyInvalidateCacheQueryString: PropTypes.string,
}

render () {
Expand All @@ -148,7 +160,8 @@ export class Main extends Component {

export class NextScript extends Component {
static contextTypes = {
_documentProps: PropTypes.any
_documentProps: PropTypes.any,
_devOnlyInvalidateCacheQueryString: PropTypes.string,
}

static propTypes = {
Expand All @@ -158,11 +171,13 @@ export class NextScript extends Component {

getDynamicChunks () {
const { dynamicImports, assetPrefix } = this.context._documentProps
const { _devOnlyInvalidateCacheQueryString } = this.context

return dynamicImports.map((bundle) => {
return <script
async
key={bundle.file}
src={`${assetPrefix}/_next/${bundle.file}`}
src={`${assetPrefix}/_next/${bundle.file}${_devOnlyInvalidateCacheQueryString}`}
nonce={this.props.nonce}
crossOrigin={this.props.crossOrigin || process.crossOrigin}
/>
Expand All @@ -171,9 +186,10 @@ export class NextScript extends Component {

getScripts () {
const { assetPrefix, files } = this.context._documentProps
if(!files || files.length === 0) {
if (!files || files.length === 0) {
return null
}
const { _devOnlyInvalidateCacheQueryString } = this.context

return files.map((file) => {
// Only render .js files here
Expand All @@ -183,7 +199,7 @@ export class NextScript extends Component {

return <script
key={file}
src={`${assetPrefix}/_next/${file}`}
src={`${assetPrefix}/_next/${file}${_devOnlyInvalidateCacheQueryString}`}
nonce={this.props.nonce}
async
crossOrigin={this.props.crossOrigin || process.crossOrigin}
Expand All @@ -206,6 +222,7 @@ export class NextScript extends Component {

render () {
const { staticMarkup, assetPrefix, devFiles, __NEXT_DATA__ } = this.context._documentProps
const { _devOnlyInvalidateCacheQueryString } = this.context
const { page, buildId } = __NEXT_DATA__
const pagePathname = getPagePathname(page)

Expand All @@ -214,12 +231,12 @@ export class NextScript extends Component {
}

return <Fragment>
{devFiles ? devFiles.map((file) => <script key={file} src={`${assetPrefix}/_next/${file}`} nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />) : null}
{devFiles ? devFiles.map((file) => <script key={file} src={`${assetPrefix}/_next/${file}${_devOnlyInvalidateCacheQueryString}`} nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />) : null}
{staticMarkup ? null : <script id="__NEXT_DATA__" type="application/json" nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} dangerouslySetInnerHTML={{
__html: NextScript.getInlineScriptSource(this.context._documentProps)
}} />}
{page !== '/_error' && <script async id={`__NEXT_PAGE__${page}`} src={`${assetPrefix}/_next/static/${buildId}/pages${pagePathname}`} nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />}
<script async id={`__NEXT_PAGE__/_app`} src={`${assetPrefix}/_next/static/${buildId}/pages/_app.js`} nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
{page !== '/_error' && <script async id={`__NEXT_PAGE__${page}`} src={`${assetPrefix}/_next/static/${buildId}/pages${pagePathname}${_devOnlyInvalidateCacheQueryString}`} nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />}
<script async id={`__NEXT_PAGE__/_app`} src={`${assetPrefix}/_next/static/${buildId}/pages/_app.js${_devOnlyInvalidateCacheQueryString}`} nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
{staticMarkup ? null : this.getDynamicChunks()}
{staticMarkup ? null : this.getScripts()}
</Fragment>
Expand Down
16 changes: 16 additions & 0 deletions test/integration/app-document/test/rendering.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,22 @@ export default function ({ app }, suiteName, render, fetch) {
expect($('#render-page-enhance-app').text().includes(nonce)).toBe(true)
expect($('#render-page-enhance-component').text().includes(nonce)).toBe(true)
})

// This is a workaround to fix https://github.com/zeit/next.js/issues/5860
// TODO: remove this workaround when https://bugs.webkit.org/show_bug.cgi?id=187726 is fixed.
test('It adds a timestamp to link tags with preload attribute to invalidate the cache (DEV only)', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a test to integration/production to make sure we don't regress there in the future.

const $ = await get$('/')
$('link[rel=preload]').each((index, element) => {
const href = $(element).attr('href')
expect(href.match(/\?/g)).toHaveLength(1)
expect(href).toMatch(/\?ts=/)
})
$('script[src]').each((index, element) => {
const src = $(element).attr('src')
expect(src.match(/\?/g)).toHaveLength(1)
expect(src).toMatch(/\?ts=/)
})
})
})

describe('_app', () => {
Expand Down
21 changes: 21 additions & 0 deletions test/integration/production/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,27 @@ describe('Production Usage', () => {
browser.close()
})

// This is a workaround to fix https://github.com/zeit/next.js/issues/5860
// TODO: remove this workaround when https://bugs.webkit.org/show_bug.cgi?id=187726 is fixed.
it('It does not add a timestamp to link tags with preload attribute', async () => {
const browser = await webdriver(appPort, '/prefetch')
const links = await browser.elementsByCss('link[rel=preload]')
await Promise.all(
links.map(async (element) => {
const href = await element.getAttribute('href')
expect(href).not.toMatch(/\?ts=/)
})
)
const scripts = await browser.elementsByCss('script[src]')
await Promise.all(
scripts.map(async (element) => {
const src = await element.getAttribute('src')
expect(src).not.toMatch(/\?ts=/)
})
)
browser.close()
})

it('should reload the page on page script error with prefetch', async () => {
const browser = await webdriver(appPort, '/counter')
const counter = await browser
Expand Down