-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
Add CDN support with assetPrefix #1700
Conversation
We no longer need it.
Because that's the place to do it.
@@ -6,7 +6,8 @@ const cache = new Map() | |||
const defaultConfig = { | |||
webpack: null, | |||
poweredByHeader: true, | |||
distDir: '.next' | |||
distDir: '.next', | |||
assetPrefix: '' |
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.
nbd, just curious: why did we move away from a function here?
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.
No any big reason. Do we really need it? Usually this is a build time config. I think it's better to have this a just a string.
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.
no, I don't think we need it. #907 had it, so I figured we'd follow suit here. I suppose since distDir
is a string, assetPrefix
should be too tho.
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.
it might be a function because we need to add the .json as well as rewriting / to /index, or does that happen automatically?
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.
the function could receive 2 parameters, the route and the buildId
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.
and returns the built url as a string, to get that resource fetched
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.
@elgubenis we don't wanna go that path.
We just wanna support typical CDNs and most of them as origin support and we don't want to re-write any URLs.
res.setHeader('Content-Type', 'text/javascript') | ||
res.end(` | ||
window.__NEXT_REGISTER_PAGE('${page}', function() { | ||
var error = new Error('Page not exists: ${page}') |
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.
nit: "Page does not exist: ${page}"
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.
Thanks. This needs to go into no-eval branch. I'll do it.
@arunoda thanks for taking this on! I lost traction on my branch, so this is hugely appreciated. ❤️ ❤️ |
This PR contains #1611 as well. |
Would there be a demo (e.g. with Now + CloudFlare) or some further doc which shows how to set this up? I haven't yet quite understood. Thanks a lot |
@sedubois see: https://www.maxcdn.com/one/tutorial/custom-cdn-integration/ But this is for CDN where you only need to serve static content. |
Fixes #737
We should take this after #1611
(Then we need to do a rebase with master)
Sometimes, it's a pretty good idea to serve Next.js static assets via a CDN. You can easily do that by exposing the following option via
next.config.js
.Then inside your CDN configuration, set the origin URL as your app's domain name. That's all you've to.