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

Lazy serve: Rebuild current opened pages on add/remove page #1550

Merged
merged 6 commits into from
Apr 22, 2021

Conversation

ryoarmanda
Copy link
Contributor

@ryoarmanda ryoarmanda commented Apr 22, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Follow up on #1513 (ref: #1513 (comment)).

After multi-tab development is established, the add/remove page flow has yet to be updated to use the current opened pages. Currently it is still rebuilding only the current viewed page.

Overview of changes:

  • Modified rebuildPageBeingViewed to adopt the page generation tasks flow
  • Changed the call of rebuildPageBeingViewed on rebuildSourceFiles to use Site.currentOpenedPages

Another problem arises, as on rebuildSourceFiles, the line after is a call to lazyBuildAllPagesNotViewed, which tests the pages against Site.currentPageViewed, so toRebuild will be refilled with almost every page, which will trigger another rebuild once live-reload starts. So, I also:

  • Modified lazyBuildAllPagesNotViewed to use a supplied viewedPages and test the pages against that instead
  • Modified any call to the above function appropriately, e.g. on lazyBuildSourceFiles to only use the current viewed page, on rebuildSourceFiles to use the current opened pages.

Anything you'd like to highlight / discuss:

Testing instructions:

With the default template from markbind init:

  • markbind serve -o
  • Aside from the index page, open another page on a new tab
  • Add a new page (easy way is to duplicate one of the files in contents)
  • Observe in the logs that both the pages opened are rebuilt

Proposed commit message: (wrap lines at 72 characters)
Lazy-serve: Rebuild opened pages on add/remove page

With the addition of the multi-tab development flow, single-page serve
can keep track of all opened pages instead of a single currently
viewed page. This flow is already adopted in page edit events,
but it has yet to be adopted in page addition or removal.

Let's adopt the multi-tab development mechanism on the page addition' +
and removal flow.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Tested and works 👍

Just some nits:

@@ -762,26 +763,23 @@ class Site {
to trigger multiple page builds before the first one has finished building,
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this comment to the original call site of the function since the use case is expanded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, moving this to the call site at changeCurrentPage

@@ -853,8 +851,8 @@ class Site {
if (this.onePagePath) {
this.mapAddressablePagesToPages(this.addressablePages || [], this.getFavIconUrl());

await this.rebuildPageBeingViewed(this.currentPageViewed);
await this.lazyBuildAllPagesNotViewed();
await this.rebuildPageBeingViewed(this.currentOpenedPages);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do:

Suggested change
await this.rebuildPageBeingViewed(this.currentOpenedPages);
await this._rebuildPagesBeingViewed(this.currentOpenedPages);

?

  • plural for the expanded use case
  • _ to use the non-delayed version (possible here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll refactor the method (and related helpers) to use plural name, and use the non-delayed version on rebuildSourceFiles (as calls to it is already delayed)

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Lgtm! 👍

@ang-zeyu ang-zeyu added this to the v3.0 milestone Apr 22, 2021
@ang-zeyu ang-zeyu merged commit ca6e988 into MarkBind:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants