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.

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

Keep some buffered pages, that won't be disposed. Fix #1939 #2592

Merged
merged 2 commits into from Sep 28, 2017

Conversation

AugustinLF
Copy link
Contributor

Implementation of @arunoda's suggestion of letting the user decide how many page should be kept in memory before disposing them.

@sedubois made his point for not over disposing built pages, so I won't repeat him, but I think he has a point:

This would allow to then have fast client-side transitions (after the first load), which allows to have a feel of these transitions without needing to deploy to prod.

The implementation is pretty naive, pagesBuffer works as a queue (first in, first out) of max length two (or whatever the user set), we keep in the buffer the last requested pages. If you'd like me to encapsulate the pagesBuffer logic in, do not hesitate, I didn't want to add any non needed abstraction :)

@arunoda
Copy link
Contributor

arunoda commented Jul 19, 2017

I really like the concept behind this PR. But I suggest to change it like this.

Check this: https://github.com/zeit/next.js/blob/v3-beta/server/on-demand-entry-handler.js#L20

It keeps tracks of pages we've accessed. Currently it only keep a single entry. Initialize that with the pageBufferLength count.

Then do a include check here: https://github.com/zeit/next.js/blob/v3-beta/server/on-demand-entry-handler.js#L237

@AugustinLF
Copy link
Contributor Author

I managed to get a working version of it, but I have a problem with the tests! The way the lastAccessPages is handled is through the middleware and the on-demand-entries-ping, which is not used in the test. Should I try to add the corresponding pings in the test suite?

@AugustinLF AugustinLF force-pushed the feature/save-two-ondemand-pages branch from b321f05 to a041b70 Compare August 7, 2017 17:02
@AugustinLF AugustinLF force-pushed the feature/save-two-ondemand-pages branch from a041b70 to 4d96366 Compare August 7, 2017 17:13
@AugustinLF
Copy link
Contributor Author

@arunoda I went around the issues I had with the tests, and manage to make it work following your suggestion (or at least, I believed it's closer from what you envisioned).

@timneutkens timneutkens changed the base branch from v3-beta to master August 27, 2017 20:15
@AugustinLF
Copy link
Contributor Author

Hey @arunoda, is this feature still something you're interested in? Would you like me to change anything? I know that with the 3.0 release, you had a pretty busy time :)

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

I'm going to merge this in 👌

@timneutkens timneutkens merged commit 3643612 into vercel:master Sep 28, 2017
arunoda added a commit that referenced this pull request Sep 28, 2017
arunoda added a commit that referenced this pull request Sep 28, 2017
arunoda added a commit that referenced this pull request Sep 28, 2017
* Revert "Keep some buffered pages, that won't be disposed. Fix #1939 (#2592)"

This reverts commit 3643612.

* Revert "Add beta installation instruction"

This reverts commit 418cc21.

* Revert "4.0.0-beta.1"

This reverts commit c2d98e2.

* Revert "Treat navigation to empty hash as hash navigate (#2971)"

This reverts commit c6bd6ef.

* Revert "React 16 (fiber) (#2996)"

This reverts commit d19cc97.
@AugustinLF AugustinLF deleted the feature/save-two-ondemand-pages branch September 28, 2017 20:31
@AugustinLF
Copy link
Contributor Author

Thanks a lot :)

@timneutkens
Copy link
Member

timneutkens commented Sep 28, 2017

It's released under @beta 👍

@marbemac
Copy link
Contributor

Sweet, any notes or additions to the readme on how to change pagesBufferLength from the default?

@AugustinLF
Copy link
Contributor Author

@marbemac My bad, here is the PR!

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants