-
Notifications
You must be signed in to change notification settings - Fork 27k
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 next export
for static builds
#1576
Conversation
…xisting plugins need to be updated. also ensure that rehydrating only gets executed once
@@ -11,3 +11,6 @@ npm-debug.log | |||
# coverage | |||
.nyc_output | |||
coverage | |||
|
|||
# osx | |||
.DS_Store |
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.
I think you should remove this, add it in your ~/.gitconfig
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.
i sort of feel like it's the project's job to prevent stuff like this from getting committed, but i don't care much either way 😄. let's see what others think
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.
Agreed with Matthew
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.
Agree it should always be in your global gitignore
Nice! 👏 You beat me to it 😊 I have a few cursory observations:
|
@herrstucki in next build refers to the root of the app, so next export should work the same. Therefore a -o, --out option would be nice for changing /build to /whatever +1, I'll fix that up
+1, but I wasn't sure how to add this. Did you get this working in your branch? It'd probably look something like this:
I ended up moving this trigger into
+1, though I think we could proceed with this PR without including that for now. I think you could set that the URLs up as aliases in the |
FWIW, I think not adding another static method to pages would be preferable. In my opinion switching to a static export should ideally require no changes to the code. Also, by adding another static method, static pages will get broken when the user forgets to add that method to the page, because In most cases (e.g. when you just do a |
So this was actually put in place so you don't need to make any changes to existing code. I've managed to export a large web app without any code changes. You can think of this lifecycle hook as something extra that you can now attach onto.
Actually it gets called on load now and then when you navigate around, so assuming your getInitialProp() functions are isomorphic, it just works™ |
I managed to do it by moving the built files to the locations where they're expected, i.e. |
Hmm, but don't you lose the "server"-provided initial props in a static build then? Wouldn't it be nice if the server and static render would be the same out of the box? 🤔 So ideally a page with // pages/foo.js
static async getInitialProps() {
return fetch('stuff.json').then(r => r.json());
} Would have the data from But you're probably right that existing implementations which rely only on |
Yah, I definitely agree that would be nice and it's originally what I was proposing, but I don't think it really works that well in practice. The server-side I'll definitely think about this some more and happy to discuss this some more, but I feel like progressive enhancement may be a better approach where you can get your site working the normal way and then call into this additional hook to bring static data into your views. I think I'd prefer less potential breakage and more verbosity. Something like this: // pages/foo.js
static async getInitialProps() {
return await fetchData()
}
static async getBuildProps() {
return await fetchData()
}
function fetchData() {
return fetch('stuff.json').then(r => r.json());
} |
Yeah, you're probably right about this. Just dropping more args into a function doesn't make it easier to use it correctly 😉 (and also impossible to typecheck). In the long term separating the different environments into separate methods (e.g. |
Sweet, I also changed |
client/index.js
Outdated
err, | ||
formatURL: exported && function (buildId, route) { | ||
route = route && route.replace(/\/$/, '') | ||
return route + '/index.json' |
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.
Maybe I'm missing something but if route
is falsy then we are returning incorrect routes like undefined/index.js
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.
Probably better rewritten as
return route
? route.replace(/\/$/, '') + '/index.json'
: '/index.json'
(also doesn't reassign the function argument)
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.
yep, good catch!
Why not passing a |
I'm not sure I see the use case of having a single app support both being hosted statically and dynamically. |
@impronunciable see discussion above? 😇 |
@herrstucki I don't agree adding new methods is the way to go, I saw the convo but it's something we should discuss further before a change as big as this one |
As far as the cognitive overload of having the |
TL;DR I'd like It's definitely a deviation, but I think it's in right direction since it allows you to introduce new hooks in a non-breaking way and each function only runs in a single context, so you don't need to consider each environment the function could be run in. The example I keep thinking about is React itself. React could be done as one big update method: function render ({ componentDidMount, componentWillUpdate }) {
if (componentDidMount) {
// ...
} else {
// render
}
} But that would get really confusing. Now I can't really envision next.js introducing many more of these lifecycle methods, so maybe it's fine to just keep it as Another consideration is |
What's the status on this? We'd love to use this internally -- I'm curious if this is something that will be merged any time soon. Is it under review? |
@matthewmueller there is now some conflicts on this PR. Hope a solution will be found (I can't really help since I am not a next.js expert). I agree that avoiding a good method seems a good idea. Will a breaking change be a real problem @impronunciable @rauchg (=> should this feature need to wait for 3.0)? |
@MoOx i'm still waiting on feedback from the questions above. i'll plan on updating the PR when there's more direction and priority. what i can say is that i've been using this branch for about a month in production and it works great, though there are still a few rough edges that need to get ironed out with the development server. the development server would need to be updated to work better with export. next export relies on |
@matthewmueller I've been using it for a little while too, working pretty well, I actually haven't hit the lifecycle issue yet. One thing I would add is the build hash for cache busting, currently it just exports a I'm now starting to look at how to add the dynamic routing to mimic the server-side api. Don't want to get too ahead but have you looked at this at all? |
@matthewmueller Thanks for creating this PR and getting export functionality working! This is awesome and very useful for things like github pages where it needs to be static. One issue I ran into though is trying to get this to play nice with non-root paths. For example, I have a project (http://tscanlin.github.io/tocbot/) and the url is not the root of the domain so right now I am fixing in a very hacky way... (https://github.com/tscanlin/tocbot/blob/master/script/fix-export.sh) But otherwise it works great locally, maybe there could be a way to export pure js files instead of json files so that eval isn't needed. Let me know if you want any help with this but it sounds like you are still waiting for some author feedback. Nice work! |
Well.. I decided I couldn't wait for this to land so I created this: https://github.com/tscanlin/next-export And I successfully deployed what I'm assuming is the first static next.js site on github pages, check it out! My script draws heavily on the work of @matthewmueller so thank you man! Cheerio.js is also amazing :) The main thing it does away with is the |
So 🎉 for the feature but 😕 for the work that went into this PR. Thanks @matthewmueller! |
@herrstucki That's not a waste. Thanks @matthewmueller for your great work. I'm closing this due to this: https://zeit.co/blog/next3-preview |
Good to hear. Sorry if I came across grumpy @arunoda. I'm very happy that this became a core feature! |
Happy to see this getting some attention, regardless of if this work was used or not. One thing:
Correct me if I'm wrong here, but it sounds like there is no way to dynamically fetch content before your page is rendered. In other words, If that's the case, it greatly reduces the usefulness of static exports. Static deploys are not only useful when your content is static, but also when you have dynamic content but desire operationally simple deployments and fewer moving parts. Will this be addressed in a future release? |
This is awesome, so glad to hear this has landed in core! I tried it out and it worked well. I had a few ideas to offer.
route = route.replace(this.assetPrefix, '');
if (!route) { route = '/' } Without this the file that's requested is like: instead of: like you'd want. What do you say @herrstucki / @arunoda any way one or both of these changes could make it in?? |
@matthewmueller shouldn't you just fetch dynamic data in |
@herrstucki
You definitely can, but then you run into a situation where the router does different things depending on how you arrive at the page. If you arrive at it via page load you'd put your fetch logic inside a Also, it's not quite a blank slate, because you have statically-rendered HTML that it serves up first. From what I can tell, the version of |
@matthewmueller but calling
Either way, |
@herrstucki yep, I'm proposing it would be called twice but in different contexts. Looks like this:
The 1st one isn't useful for loading anything dynamic (just building your application shell or static content), the 2nd one is useful for loading dynamic data (like user data). What I'd like to see is some way to do 2. in a way that's consistent with client-side routing. I'm not partial to the way in which this is achieved, just that it's possible to do. /cc @arunoda |
@herrstucki @matthewmueller In our next-export implementation, it works exactly as how it's works normally with Next.js. getInitialProps runs when running next-export as it's similar to server rendering of the page. When the page loads, getInitialProps won't run. But when you are doing a client side routing it will. |
@matthewmueller but isn't your 2nd point already solved with my 2nd point (using React's lifecycle)? That's what you'd do in a static app without next.js as well, right? |
It's worth a try, but imagine you have two pages where you need to load user data and you have a If you do all the loading in It also makes the router pretty useless for all static builds, which makes next.js less of a win. |
Since it doesn't use
|
@vysakh0 |
@herrstucki is there any way we can change the behavior to delete all the files under the export directory but not the export directory itself? |
@tscanlin I think that's a great idea but I'm not actually a contributor to next.js – I've just been particularly interested in this feature 😅 So probably the best way to get this in is to make a PR on the |
Haha, no worries, thanks! |
This PR adds support for a
next export -o <out> <dir>
command and closes #604. This PR is a non-breaking change and intentionally makes very few changes to the existing codebase.The way it works is it first performs a
next build
and then takes that output and turns it into a self-executing static bundle. The lifecycle hooks in a static build are a bit different than a dynamic build:getStaticInitialProps()
static method if they have one. This will enable things like pulling in and compiling markdown files at build time. Any errors that happen here will halt the build. The reason to go withgetStaticInitialProps()
as opposed togetInitialProps({ build: true })
is thatgetStaticInitialProps()
won't break any existing next.js plugins (ex. https://github.com/matthewmueller/next-cookies/blob/master/index.js#L20-L28) and can be treated as a progressive enhancement.getInitialProps()
on page load. This is a bit different because in the dynamic build, the server will callgetInitialProps()
, but since we don't have control over the server in static builds, we'll call it here. This is where you'll load user data.I made 4 changes to the existing codebase:
__NEXT_DATA__
will now receive{ exported: true }
so it can adjust the way the router fetches new pages.Document
now knows about{ exported: true }
to adjust the location of theapp.js
common bundle.{ exported: true }
from__NEXT_DATA__
, we also triggergetInitialProps
on page load before rehydrating (as mentioned above).export
command inbin/next
There was a lot of discussion in #604 about custom routing. I decided not to include custom routing in this PR because most static file servers support redirects and by using the
as
parameter in<Link as={as} />
andRouter.push(url, as)
, you can support dynamic routes like/blog/:slug
.Wasn't sure how to setup the test environment for this project, but happy to add tests once the implementation is settled on.
Note that there's a couple
TODO
s inserver/export.js
. Please have a look as I wasn't sure if those pieces are relevant for production or if they're just used in development.Let me know what you think and if there's anything I can do to improve this PR!