Invalidate cache for link[preload] in DEV #6183

giuseppeg · 2019-01-31T11:20:08Z

giuseppeg · 2019-01-31T11:26:01Z

test/integration/app-document/test/rendering.js

+      // 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 () => {
+        const $ = await get$('/')
+        $('script, link[rel=preload]').each((index, element) => {


Actually I am not setting that on script tags as adding the timestamp to the link tags only seems to be fine. That said probably it makes sense to add the timestamp to the script tags as well.

timneutkens

Actually only adding it to preload defeats the purpose of preload right 🤔 as it would be 2 unique requests

giuseppeg · 2019-01-31T13:57:03Z

Actually only adding it to preload defeats the purpose of preload right 🤔 as it would be 2 unique requests

yes I think so, I'll try to get the npm link-ing work later and try it out.

giuseppeg · 2019-01-31T14:12:55Z

@timneutkens would it make sense to map over the head and body children, look for links with preload tags and then add the timestamp to both them and the matching script tags? Or do you prefer the hardcoded version?

giuseppeg · 2019-01-31T15:17:18Z

alright it should be better now :)

timneutkens · 2019-01-31T17:38:13Z

@giuseppeg I actually think you could add the timestamp on the renderer level, next-server/server/render.tsx, as then you don't even have to manage adding it in multiple places 👍

giuseppeg · 2019-01-31T19:52:13Z

oh yeah definitely! Sorry I am not that familiar with the codebase yet 😅

timneutkens · 2019-01-31T20:01:28Z

@giuseppeg no worries 🙌 This would actually be the correct solution on older versions of Next.js, that bundle rendering was added in v6

giuseppeg · 2019-02-01T14:27:50Z

packages/next/pages/_document.js

-      <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_app.js`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
-      <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_error.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}${this.preloadLinkTagInvalidate}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />}
+      <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_app.js${this.preloadLinkTagInvalidate}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />


@timneutkens I just realized that these are hardcoded paths so I can't add the timestamp in next-server/server/render.tsx.
Either we add these there or keep this latest revision. I can cleanup by doing the string concatenation with a function.

@timneutkens what is your preference here? Should we stick to the current revision or should I move (hardcode) these files in next-server/server/render.tsx and go for your suggestion? There are pro and cons with both but I don't have a strong preference.

Let's keep it like this

giuseppeg · 2019-02-01T14:28:17Z

packages/next/pages/_document.js

-      <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_error.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}${this.preloadLinkTagInvalidate}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />}
+      <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_app.js${this.preloadLinkTagInvalidate}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
+      <link rel='preload' href={`${assetPrefix}/_next/static/${buildId}/pages/_error.js${this.preloadLinkTagInvalidate}`} as='script' nonce={this.props.nonce} crossOrigin={this.props.crossOrigin || process.crossOrigin} />
      {this.getPreloadDynamicChunks()}
      {this.getPreloadMainLinks()}


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

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

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

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

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

timneutkens · 2019-02-01T18:41:04Z

packages/next/pages/_document.js

@@ -16,7 +16,7 @@ export default class Document extends Component {
  static getInitialProps ({ renderPage }) {
    const { html, head } = renderPage()
    const styles = flush()
-    return { html, head, styles }
+    return { html, head, styles, timestamp: Date.now() }


Can we disable this in prod, probably not too heavy but still not useful to have Date.now running on every request.

timneutkens · 2019-02-01T18:44:48Z

test/integration/app-document/test/rendering.js

+
+      // 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 () => {


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

giuseppeg · 2019-02-01T19:40:04Z

packages/next/pages/_document.js

+    // 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.
+    if (process.env.NODE_ENV !== 'production') {
+      initialProps.timestamp = Date.now()


a type checker would yell at me so badly here 😅

timneutkens · 2019-02-03T00:04:36Z

packages/next/pages/_document.js

@@ -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,


Should this use React 16.3 context instead?

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

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.

Invalidate cache for link[preload] in DEV (vercel#6183)

giuseppeg commented Jan 31, 2019

View reviewed changes

timneutkens requested changes Jan 31, 2019

View reviewed changes

giuseppeg commented Feb 1, 2019

View reviewed changes

timneutkens reviewed Feb 1, 2019

View reviewed changes

giuseppeg commented Feb 1, 2019

View reviewed changes

giuseppeg added 2 commits February 1, 2019 20:44

Invalidate cache for link[preload] in DEV

6d230b7

Add timeout to scripts src too

746670d

giuseppeg force-pushed the fix-dev-safari-caching-issues branch from b396437 to da0cf2f Compare February 1, 2019 19:45

Do not define timestamp in prod, add prod testcase

e8b53a0

giuseppeg force-pushed the fix-dev-safari-caching-issues branch from da0cf2f to e8b53a0 Compare February 1, 2019 19:48

giuseppeg added 2 commits February 2, 2019 10:01

Remove duplication

12e7e05

add query string safeguard test

5612ffb

timneutkens reviewed Feb 3, 2019

View reviewed changes

timneutkens approved these changes Feb 3, 2019

View reviewed changes

timneutkens merged commit 5a4176c into vercel:canary Feb 3, 2019

rajendraarora16 added a commit to rajendraarora16/next.js that referenced this pull request Feb 3, 2019

Merge pull request #53 from zeit/canary

842d15f

Invalidate cache for link[preload] in DEV (vercel#6183)

lock bot locked as resolved and limited conversation to collaborators Feb 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalidate cache for link[preload] in DEV #6183

Invalidate cache for link[preload] in DEV #6183

giuseppeg commented Jan 31, 2019

giuseppeg Jan 31, 2019

timneutkens left a comment

giuseppeg commented Jan 31, 2019

giuseppeg commented Jan 31, 2019 •

edited

Loading

giuseppeg commented Jan 31, 2019

timneutkens commented Jan 31, 2019

giuseppeg commented Jan 31, 2019

timneutkens commented Jan 31, 2019

giuseppeg Feb 1, 2019 •

edited

Loading

giuseppeg Feb 1, 2019

timneutkens Feb 1, 2019

giuseppeg Feb 1, 2019

timneutkens Feb 1, 2019

timneutkens Feb 1, 2019

giuseppeg Feb 1, 2019

timneutkens Feb 1, 2019

timneutkens Feb 1, 2019

giuseppeg Feb 1, 2019

timneutkens Feb 1, 2019

giuseppeg Feb 1, 2019

timneutkens Feb 3, 2019 •

edited

Loading

timneutkens Feb 3, 2019

giuseppeg Feb 3, 2019

Invalidate cache for link[preload] in DEV #6183

Invalidate cache for link[preload] in DEV #6183

Conversation

giuseppeg commented Jan 31, 2019

Choose a reason for hiding this comment

timneutkens left a comment

Choose a reason for hiding this comment

giuseppeg commented Jan 31, 2019

giuseppeg commented Jan 31, 2019 • edited Loading

giuseppeg commented Jan 31, 2019

timneutkens commented Jan 31, 2019

giuseppeg commented Jan 31, 2019

timneutkens commented Jan 31, 2019

giuseppeg Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timneutkens Feb 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppeg commented Jan 31, 2019 •

edited

Loading

giuseppeg Feb 1, 2019 •

edited

Loading

timneutkens Feb 3, 2019 •

edited

Loading