Lazy serve: Track opened pages for dependency changes #1513

ryoarmanda · 2021-03-10T19:19:23Z

What is the purpose of this pull request?

Addresses the first enhancement proposal on #1405

Overview of changes:

Patched the live-server library to expose current active tabs' urls, in order from most-to-least recently visited.
Added a property currentOpenedPages to the Site class, which is an array that will store the absolute extension-less path to the current opened pages, ordered in the most-to-least recently visited.
Added a Site method changeCurrentOpenedPages to change the current opened pages list according to the given array of normalized URLs, filtering out duplicates and keeping the first match (i.e. the most recently visited among the duplicates)
Modified the current viewed page check in regenerateAffectedPages to check whether the page is on the current opened pages list. Also modified page regeneration by prioritizing opened pages, of which is ordered in the same order as the current opened pages list.
To support the above point, added a utility function and methods to perform sequential page generation.
Added instruction to synchronize the current opened pages list with the active tabs' urls from live-server on file add/change/remove handlers.

Anything you'd like to highlight / discuss:

Testing instructions:
Test on the docs folder for convenience. Refer to the case table on the below comment #1513 (comment).

Proposed commit message: (wrap lines at 72 characters)
Lazy serve: Track opened pages for dependency changes

The lazy serve option tracks only the last visited page for changes
in its dependencies. Users editing multiple files must manually
reload each page to keep the browser synchronized with the content.

Let's track the opened pages instead, and rebuild them when any of
its dependencies have changed.

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!

wxwxwxwx9 · 2021-03-12T06:15:44Z

Hi @ryoarmanda! Just some personal feedback about how to improve user experience. Suppose we have a scenario where I have visited >5 pages, with >5 tabs open and I can't recall which pages are the top 5 most recently visited. And suppose I happen to modify the content of the page which it is not in the top 5 most recently visited list, without being aware of it.

This may confuse the user as to why the page wasn't updated. Furthermore, the user has no easy way to find out which of the pages are currently being tracked for reloading (in the top 5 list).

Thus, I think it may be a good idea to have a logging feature to update the user of the updated list of top 5 most recently visited pages; logged in the console whenever the user visits a new page, or when he modifies any pages (just to make sure that the list of top 5 most recently visited pages doesn't get lost in the logs).

Also can I check what are the benefits of having a limit on the most recently visited pages? I was thinking what if we explore not having a limit on the most recently visited pages -- so when a page will be tracked for rebuilding if the user has visited it before? What do you think about this? @ryoarmanda :-)

ryoarmanda · 2021-03-12T10:02:54Z

Thus, I think it may be a good idea to have a logging feature to update the user of the updated list of top 5 most recently visited pages; logged in the console whenever the user visits a new page, or when he modifies any pages (just to make sure that the list of top 5 most recently visited pages doesn't get lost in the logs).

Yeah I agree with the concern of identifying the recently visited pages too. I will add the log for developer convenience.

Also can I check what are the benefits of having a limit on the most recently visited pages? I was thinking what if we explore not having a limit on the most recently visited pages -- so when a page will be tracked for rebuilding if the user has visited it before?

I essentially followed how markbind serve -o works so far, I just extended it to only rebuilding when the user recently visits that page and some of its dependencies have changed, to keep it "lazy" like the original. In my opinion, if we don't put a limit on how far back should we keep track, there can be a case MarkBind rebuilds a page that was visited only once a long time ago, which I feel is not in line with the laziness aspect (should rebuild only when user really wants it to) and/or does some unnecessary processing time (especially if that page is big).

While we don't know the real intent of the user (i.e. how do we know when the user wants this page to be rebuilt?), I argue we can sort of make a guess on their "current workspace" based on their several most recently visited pages, hence why I put a limit to know which of the visited pages are still relevant to the user right now. A more accurate way to gauge the workspace is by checking out the opened tabs of the browser, but so far I don't see any good way to do that.

damithc · 2021-03-12T10:35:02Z

Is this going to affect the performance of the live preview e.g., the speed of rebuilding?

ryoarmanda · 2021-03-12T12:46:10Z

Is this going to affect the performance of the live preview e.g., the speed of rebuilding?

I don't think there's any performance difference in the rebuilding side as I didn't modify any of the page generation functions. The changes here is more on the part which identifies whether a page should be rebuilt when there are file changes detected, in which I feel is only a slight increase if there's any (the original compare against 1 page path vs the new compare against up to 5 page paths)

ang-zeyu · 2021-03-12T14:23:20Z

Thanks! @ryoarmanda

I don't think there's any performance difference in the rebuilding side as I didn't modify any of the page generation functions. The changes here is more on the part which identifies whether a page should be rebuilt when there are file changes detected, in which I feel is only a slight increase if there's any (the original compare against 1 page path vs the new compare against up to 5 page paths)

quick comment on this aspect first (reviews from other folks still very welcome):

it has a rather noticeable effect (try editing panels.mbdf in the docs in markbind s (4 pages - 2.41s) vs markbind s -o (0.67s))

you'll likely need to make some changes + additions to the page generation queueing logic to fufill the second part in #1405 as well to avoid this (note simply ordering according to recentlyViewedPages won't work because there's quite a few async operations in page generation)

then rebuild these pages in order of most-recent when any of their dependencies change

ryoarmanda · 2021-03-12T16:37:05Z

it has a rather noticeable effect (try editing panels.mbdf in the docs in markbind s (4 pages - 2.41s) vs markbind s -o (0.67s))

Sorry, can I clarify whether you mean that the performance is off compared to master's performance? 😅 My earlier comment was about that, in the sense that there should be no noticeable change compared to the current performance as my change was just revolving on the decision whether to rebuild and not rebuilding itself (though by the comment you made that reordering does not matter actually realized that my reordering part is redundant =x I misunderstood the comment on generatePagesThrottled, thought the pages are generated sequentially).

If you do mean that the performance is off, I just tried benchmarking the changes here against master and the timing is pretty much the same. On editing panels.mbdf and viewing /userGuide/usingComponents.html#panels, with my machine, serve takes 3.3s average and serve -o takes 0.5s average for both branches.

ryoarmanda · 2021-03-12T16:51:08Z

you'll likely need to make some changes + additions to the page generation queueing logic to fufill the second part in #1405 as well to avoid this

I'm currently thinking that as we want to achieve sequential generation for recently viewed pages (so it generates in order from most-to-least recent), and async generation on pages that do not care about order of generation (to leverage the async operations), I might be able to modify the generatePagesThrottled method such that it can generate pages according to the situation. Like it can receive an array of objects, of which it contains the "mode" that we want ("sequential" or "async"), and the array of pages to be generated under that mode.

ang-zeyu · 2021-03-12T16:56:55Z

the original compare against 1 page path vs the new compare against up to 5 page paths

Ahh, thanks for clarifying. I was commenting on this part in particular, but I may have misinterpreted it - in that building 5 pages simultaneously would simply be less performant than one. You'll need to do

await generate page 1
await generate page 2
...

// instead of
generate page 1, generate page ..., generate page 5 (capped at 4 with the existing current logic)
await all generation

though by the comment you made that reordering does not matter actually realized that my reordering part is redundant =x

the ordering you have here already isn't redundant, just needs to be further built upon

If you do mean that the performance is off, I just tried benchmarking the changes here against master and the timing is pretty much the same. On editing panels.mbdf and viewing /userGuide/usingComponents.html#panels, with my machine, serve takes 3.3s average and serve -o takes 0.5s average for both branches.

You'll need to visit (start tracking) all the pages which has panels.mbdf as a dependency too. (e.g. fullSyntaxReference.md)

damithc · 2021-03-12T17:44:06Z

My worry is that this will slow down for live preview experience for authors across the board. Some of the pages in my MarkBind sites are quite heavy, and building 5 instead of 1 can slow me down noticeably.

ang-zeyu · 2021-03-13T03:18:27Z

My worry is that this will slow down for live preview experience for authors across the board. Some of the pages in my MarkBind sites are quite heavy, and building 5 instead of 1 can slow me down noticeably.

It should be ok if we adopt sequential generation of those 5 pages (so the most recent gets updated first, no slow down in how fast it takes for the updated version to show on the most recent tab).

Its also quite likely not all of the 5 most recently visited pages depend on the file that updated, so in practice only 2 / 3 pages should be sequentially built most of the time. (assuming you open multiple tabs to work on mostly "closely related" pages - which may not even be true)

The overall system performance would still take a hit, which is unavoidable. It can be mitigated by some smarter scheduling (second part of #1405), but that's a substantial change we can put that off to another PR, unless @ryoarmanda is up for integrating that here right away. 😁 (sequential generation should really suffice for now though)

On recently viewed pages limit: I put 5 as a baseline. What do you think? Is it okay, or do you think it should be increased/decreased?

5 sounds good to me. Any thoughts? @damithc

damithc · 2021-03-13T03:27:36Z

5 sounds good to me. Any thoughts? @damithc

Yes, we can go with 5 and see how it performs in practice.

It should be ok if we adopt sequential generation of those 5 pages (so the most recent gets updated first, no slow down in how fast it takes for the updated version to show on the most recent tab).

Its also quite likely not all of the 5 most recently visited pages depend on the file that updated, so in practice only 2 / 3 pages should be sequentially built most of the time. (assuming you open multiple tabs to work on mostly "closely related" pages - which may not even be true)

The overall system performance would still take a hit, which is unavoidable. It can be mitigated by some smarter scheduling (second part of #1405), but that's a substantial change we can put that off to another PR, unless @ryoarmanda is up for integrating that here right away. 😁 (sequential generation should really suffice for now though)

Got it. Thanks for the explanation @ang-zeyu

ryoarmanda · 2021-03-13T14:08:35Z

Ahh, thanks for clarifying. I was commenting on this part in particular, but I may have misinterpreted it - in that building 5 pages simultaneously would simply be less performant than one.

Oh I wasn't meaning to say that 😅 what I meant was that now there's a comparison against (up to) 5 page paths on determining whether a page should be rebuilt, from the original check that compares against 1 path (the currentPageViewed). Sorry that my wording got you confused haha.

The overall system performance would still take a hit, which is unavoidable. It can be mitigated by some smarter scheduling (second part of #1405), but that's a substantial change we can put that off to another PR, unless @ryoarmanda is up for integrating that here right away. 😁 (sequential generation should really suffice for now though)

Yeah performance is a bit varied now with sequential generation, sometimes better and other times slower than the serve rebuilds (referring to the 4 page rebuild case you mentioned), but as you said, it's rare that you get more than 2/3 recently viewed page rebuilds. I might do it in another PR when I have an idea for it, still thinking of a better way to schedule everything.

ryoarmanda · 2021-03-13T16:43:03Z

There's a hiccup on maintaining the recently viewed pages list which I have added the countermeasure just now.

From how the live-server library is implemented, whenever it detects file changes, it will issue a reload to all opened tabs, effectively spamming the server with GET requests for every page that is currently opened (not necessarily the ones who are recently visited, could be a tab that was there for a long time), which can mess up the list.

My best idea for a countermeasure is that we classify it based on time since the last request. My observation is that reload spam requests tend to be very close with each other (only around 5-10ms between each request), page redirects tend to be close (80-100ms), and intentional page visits are pretty much above that. I put a threshold of 75ms such that we can allow redirects and intentional page visits to be recorded to the recently viewed pages list, while reload spam requests are kept out of it.

But admittedly it's not perfect. For example, it is hard to distinguish a request that came from user edits a file after 12s of idle, starting a reload spam vs user visits a new page after 12s of idle. Both cases result in a request in which it has 12s of time since last request, but we don't want to add the former to the recently viewed pages, while the latter should be added. However, that impact of that edge case is pretty small, probably only changing the most-recently visited page record to be whatever the first websocket/tab that is stored in live-server (see Aside below).

As far as I can think of, I can't distinguish the two from what we have right now 😓. Tried an idea of having a flag that is set to true whenever the file handlers are fired, but we don't know how long the reload spam is going to take, so don't know when to set down the flag again. If anyone has any other ways to handle this, I'm all ears :)

Aside: This prompts me to look for the implementation of live-server. Apparently it locally stores a list of client websockets that corresponds to the exact tabs opened in the browser. It's an accurate measure of the user's workspace, but sadly, it's not exported for external use.

ang-zeyu · 2021-03-14T07:01:46Z

Thanks for investigating this.

As you highlighted, I don't think we should be relying on indeterministic time-sensitive behaviour for the end user, which can vary very greatly from system to system, background processes, ...

Would it be possible to do a simple patch for the purposes here?
Or fork (if it makes more sense) the entire live-server implementation. It may help out in fixing the cause of #1507 as well, also any future new shiny features requiring modifications to the server. (but I think a simple patch would be preferable if possible for now, we can fork as needed in the future if warranted)

ryoarmanda · 2021-03-14T08:25:37Z

Would it be possible to do a simple patch for the purposes here?
Or fork (if it makes more sense) the entire live-server implementation. It may help out in fixing the cause of #1507 as well, also any future new shiny features requiring modifications to the server. (but I think a simple patch would be preferable if possible for now, we can fork as needed in the future if warranted)

I think I might have to do a fork, as for the reloads and the local websocket storage is directly implemented in the start method of live-server. Among other things, that method implements a lot of specific stuff such as server setup, configuration, and so on. If I do a patch of that method, I probably need to copy the whole implementation, which is not ideal as that includes something that we shouldn't really handle with our hands for now.

(For reference, the websocket management part on the start method is here: https://github.com/tapio/live-server/blob/01cd7f970439d42537bbe13b4885befb816a0446/index.js#L315-L342)

ryoarmanda · 2021-03-18T05:29:37Z

@ang-zeyu @wxwxwxwx9 @damithc I just updated the strategy of determining recently viewed pages by patching the live-server library to expose the active URLs from the server's side (in order from most-to-least recent), and on MarkBind's side it filters out the duplicates and store it as a Site property that will be used to determine the order of sequential generation.

This list will be updated whenever a request has finished and is not a request that comes from live reload (the patch also exposes this utility function to test whether a given request URL corresponds to a live reload). In other words, the list will be updated whenever a new page is visited by user, either by intention or redirect. Also as we have the exact measure of the user's workspace, there is no need to cap the amount of pages to be considered recently viewed.

There are lots of edge cases that can happen here mainly due to asynchronicity of requests and connection closes, and I feel like I have addressed most of them here, but I am not sure if I have caught all the cases. I'll try documenting all the cases I have found later on, but if anyone is available to play around that will be very helpful :)

ryoarmanda · 2021-03-18T13:47:35Z

Just for documenting for anyone who wants to test the opened pages list, here are the cases I discovered and addressed:

No	Cases	Expected behaviour
1	Visits a fresh page on a new tab	Opened pages list is updated when the first request finishes, the visited page URL is at the top of the list. Following live reload requests does not affect the opened pages list.
2	Visits a built page on a new tab	Opened pages list is updated, the visited page URL is at the top of the list.
3	Visits a page that actually just a redirect to another page (e.g. our docs' `userGuide/index`)	Opened pages list is updated twice, the first adds the original page URL to the top of the list, the next removes that URL and adds the redirected page URL to the top of the list
4	Visits a page when the same page is also opened on another tab	Opened pages list is updated, for any duplicate URLs, only keep the first match (i.e. most-recently opened among them)
5	Reload a tab	Opened pages list is updated, the reloaded page URL is moved to the top of the list.
6	Close a tab	Opened pages list is not affected (see Note), but the changes (i.e. the page URL is removed from the list) will reflect on next event that will update the opened pages list.
7	Visits a page on an existing tab	Opened pages list is updated, the new page URL is at the top of the list, but the old page URL is still present (see Note)
8	Edit a page	Opened pages list is synchronized before issuing rebuild to address cases that are affected by Note (Case 6 and 7)

Note: On the live-server side, tab/connection close is recognized when the client socket established is closed, but this socket communication flow is established after the request has finished, as it is the product of the library's injected script in the sent HTML. MarkBind can only listen to as far as request finish, so as far as I can research MarkBind cannot actually react to a tab/connection close event in real-time. Aside from that, when a tab closes, the live-server patch will handle accordingly so that when MarkBind calls getActiveUrls on the next event, the closed tab URL is already removed.

ang-zeyu

Nice work! 👍

Let me confirm my understanding of your patch here first:

activeTabs is only for page regeneration

if there is a non-live reload request, push a new activetab - ~~this is for new page navigations~~
- done here instead of at socket time since you can't open the socket without the page built (on the earlier point for regeneration, if so, could this stay at socket initialisation time? (3))
On file change / addition / removal (caused by a page rebuilt, new page built, page deleted), flag the activetab object as isReloading, "update" client / prevClient, send the 'reload' / refreshcss event as per before to all tabs
- for the reload event, it results in the page reloading, so the old websocket pair is closed; a new one is opened, as per before
- unlike before though, closing the websocket now only removes from activeTabs the activeTabs which have client === theClosedSocket
  - i.e. only socket closures not resulting from live reloads would remove the active tab (let's document it if so)
On socket establishment, update the corresponding activeTab from (1) with the client
- is it possible for the tab to ever not exist? (in which case if (tab) { is unnecessary)
When any request-response finishes, update Site's "copy of activeTabs"
- new page builds (page navigations) is still handled by the old code

ang-zeyu · 2021-03-18T12:42:23Z

packages/cli/src/lib/live-server/index.js

+require('colors');
+
+// CHANGED: added relative path that directs to the live-server directory from here
+const relativePathToLiveServer = "../../../../../node_modules/live-server";


could use require.resolve here so it works when published too

Oh yes, will do.

ang-zeyu · 2021-03-18T12:43:22Z

packages/cli/src/lib/live-server/index.js

+ * This patch allows us to gain access to the information that can be gathered with the client
+ * websockets, which in turn can enables the support for multiple-tab development.
+ *
+ * Patch is written against live-server v1.2.1


let's fix the version in package.json just in case

…-server

ang-zeyu · 2021-03-19T05:39:50Z

If you mean for MarkBind's usage, I guess that's true, as I think the old code only concerns with "check whether the page is the current viewed one" issue on regenerating affected pages.

Generally yes, but updating the copy only happens when the request does not come from a live-reload. Can I clarify what you mean by your sub-point? If you mean by the new-page build flow (changing the url to the spinning wheel file, then wait for live-reload), yeah it's largely untouched.

yup, just the spinning wheel builds.

As an aside, just to cover my bases, I also added synchronizing the Site's copy on the MarkBind's add and remove handlers as well, I feel it would be nice to keep the copy accurate with the patch on any events.

In that case, is the getActiveUrls / changeCurrentOpenedPages call on res.on('close') still necessary? Since this handles page regeneration, should only need to update it on MarkBind's chokidar fs events.

Unfortunately we can't put it at socket initialisation. In short, MarkBind is not "aware" of what happens around sockets as it happens beyond the request-response communication. The socket initialisation is actually initiated from the browser via live-server's injected script as it displays the page. In order for MarkBind to record the opened pages properly, we need to find a way to record an activeTab before the request-response is finished, and thus I opted to do it in the server's handler. I put it specifically at that part (in the file handler) as all HTML requests will go through there. The remaining activeTab data (client) will only be filled in when the socket initialisation starts.

Thanks for the clarification. 👍 Let's try to make sure this is as valid as best as possible since this is pretty complex.

To confirm again, so this is to take care of the case the user edits the same file they just navigated to between the considerable time lag of a request <-> socket establishment?

Would this cause issues if:

user navigates to new page
immediately closes the tab (before the socket is established)
the activeTab object remains in activeTabs forever since there is no socket to close it
? (but if so that's ok, I think the former is better in this tradeoff, the latter is highly unlikely)

Yeah that's right, I'll add it to the changed comment message. Also, just realized that if check before removing the activeTabs is unnecessary, so I'll remove that as well.

Not really, I believe it's guaranteed for tab to exist because in order to be there must be a corresponding activeTab that has the same url but has no client (as both non-live-reload and live-reload requests were ensured to possess that property before sending a response). I added the guard just in case, but yeah maybe we don't need it.

👍

aside: do push incrementally so its easy to see what's changed, or with a fixup strategy if you want to keep your working commits tidy (#1478 (comment)) 😅

ryoarmanda · 2021-03-19T18:23:48Z

To confirm again, so this is to take care of the case the user edits the same file they just navigated to between the considerable time lag of a request <-> socket establishment?

Initially I did it that way so MarkBind can grab the active urls to be able to have a real-time report of the current opened pages to the user when they visited a page (inspired by the earlier comment in this PR #1513 (comment) about the risk of authors' difficulty to precisely know what pages are the recently visited), but it also goes along with the fact that there is a gap between request and socket establishment, so this way it will properly record the page is opened even though the live reload has not been established.

Aside, not sure if real-time feedback is really necessary as we will sync and log the opened pages list during MarkBind's fs event handlers anyway, what do you think? If it is unnecessary, I think we can remove the res.on('close') listener in lazyReloadMiddleware.

Would this cause issues if:

user navigates to new page

immediately closes the tab (before the socket is established)

the activeTab object remains in activeTabs forever since there is no socket to close it
? (but if so that's ok, I think the former is better in this tradeoff, the latter is highly unlikely)

Yes, this can unfortunately happen, the activeTab object will remain there forever as you described, which in turn cause that quickly-closed page url to be included in the selection for pages to be sequentially generated (if that page url wasn't supposed be there). But, I think, it is a very small window of time to be able to do this. I've been trying it out now and I had to be quick in immediately opening and closing the page.

Off the top of my head, I think it's also quite hard to find a way minimize the impact of this edge case without causing other unintended effects. Thought of filtering out activeTabs that is not reloading and does not have a client when the live-server's fs event handler is fired so future calls to getActiveUrl will have the corrected copy, but I don't know if I can ensure that all still-opened activeTabs already have a client established when the call happens.

Would it be okay for us to have the limitation as you described?

ang-zeyu · 2021-03-20T03:34:28Z

In that case, is the getActiveUrls / changeCurrentOpenedPages call on res.on('close') still necessary? Since this handles page regeneration, should only need to update it on MarkBind's chokidar fs events.

Aside, not sure if real-time feedback is really necessary as we will sync and log the opened pages list during MarkBind's fs event handlers anyway, what do you think? If it is unnecessary, I think we can remove the res.on('close') listener in lazyReloadMiddleware.

Yes, let's remove as mentioned ^. Shouldn't be necessary with the syncing on fs events.

Yes, this can unfortunately happen, the activeTab object will remain there forever as you described, which in turn cause that quickly-closed page url to be included in the selection for pages to be sequentially generated (if that page url wasn't supposed be there). But, I think, it is a very small window of time to be able to do this. I've been trying it out now and I had to be quick in immediately opening and closing the page.

Off the top of my head, I think it's also quite hard to find a way minimize the impact of this edge case without causing other unintended effects. Thought of filtering out activeTabs that is not reloading and does not have a client when the live-server's fs event handler is fired so future calls to getActiveUrl will have the corrected copy, but I don't know if I can ensure that all still-opened activeTabs already have a client established when the call happens.

Would it be okay for us to have the limitation as you described?

Don't think its worth the effort currently either.

Should we document this as a todo though? Maybe someone can find a way to fix this up later. =P

ang-zeyu · 2021-03-20T03:35:05Z

packages/core/src/utils/index.js

+    for (let i = 0; i < array.length; i += 1) {
+      // eslint-disable-next-line no-await-in-loop
+      await func(array[i]);
+      // eslint-enable-next-line no-await-in-loop


Yeah, I think this is kind of unavoidable. We need to await the current async function to finish before moving onwards to the next element so that the async functions is executed in order. forEach is not able to support this kind of behaviour so I need to create a new approach. Had to make do with disabling that eslint rule for my approach.

Reference:

https://stackoverflow.com/questions/37576685/using-async-await-with-a-foreach-loop

https://codeburst.io/javascript-async-await-with-foreach-b6ba62bbf404

referring to the eslint-enable-next-line in particular

Oh god haha my mistake 😅 TIL I don't need to do that

ryoarmanda · 2021-03-20T07:04:51Z

Don't think its worth the effort currently either.

Should we document this as a todo though? Maybe someone can find a way to fix this up later. =P

Certainly, hope someone comes around and found a solution to this 😄 I'll write a todo at the activeTabs insertion/filtering phase.

ang-zeyu

Thanks, nice work on documenting the test cases as well.

Couple last ones:

ang-zeyu · 2021-03-20T12:53:54Z

packages/core/src/Site/index.js

+   * @param {Array<string>} normalizedUrls Collection of normalized url of pages taken from the clients
+   * ordered from most-to-least recently opened
+   */
+  changeCurrentOpenedPages(normalizedUrls) {


needs a guard against non-lazy serve mode

ang-zeyu · 2021-03-20T12:59:21Z

packages/cli/src/lib/live-server/index.js

+
+        // CHANGED: Added line to create new entry on non-live-reload requests
+        const reqUrl = req.originalUrl;
+        if (!LiveServer.isLiveReloadRequest(reqUrl)) {


needs one more guard against xx._include_.html types of files (dynamically loaded ones used in pages but not actually pages)

Ah right, will do.

ang-zeyu · 2021-03-20T13:12:21Z

packages/core/src/utils/index.js

+    for (let i = 0; i < array.length; i += 1) {
+      // eslint-disable-next-line no-await-in-loop
+      await func(array[i]);
+      // eslint-enable-next-line no-await-in-loop


referring to the eslint-enable-next-line in particular

ang-zeyu

Lgtm 👍

ang-zeyu · 2021-03-20T15:08:04Z

Commit message needs an update though

ryoarmanda · 2021-03-20T15:34:08Z

Right, I have updated the commit message in the original PR comment 👍

ang-zeyu · 2021-03-21T14:50:51Z

@ryoarmanda just noticed this

Reproduction:

markbind s -d -o
click User Guide in topnav
press back button in browser (chrome/firefox)

Unfortunately we can't put it at socket initialisation. In short, MarkBind is not "aware" of what happens around sockets as it happens beyond the request-response communication. The socket initialisation is actually initiated from the browser via live-server's injected script as it displays the page. In order for MarkBind to record the opened pages properly, we need to find a way to record an activeTab before the request-response is finished, and thus I opted to do it in the server's handler. I put it specifically at that part (in the file handler) as all HTML requests will go through there. The remaining activeTab data (client) will only be filled in when the socket initialisation starts.

We might have to shift the active tab initialisation to socket initialisation time after all.

PS: I didn't quite get the part above in bold earlier though (my pov is the things you added under CHANGED: Added line to record tab client socket on creation is basically it. Also the error message, and ws.onopen), and interpreted the benefit as the one below and further below. Might be missing something though 🙈.

To confirm again, so this is to take care of the case the user edits the same file they just navigated to between the considerable time lag of a request <-> socket establishment?

There is this, but just realised shouldn't need to be concerned with this case as well, because the socket is also initialised while the page is building (in the spinning wheel page).

Initially I did it that way so MarkBind can grab the active urls to be able to have a real-time report of the current opened pages to the user when they visited a page (inspired by the earlier comment in this PR #1513 (comment) about the risk of authors' difficulty to precisely know what pages are the recently visited)

on the same lines, since the socket is almost instaneously initialised, shouldn't be necessary

ryoarmanda · 2021-03-22T17:35:24Z

@ang-zeyu Thanks for catching the back edge case! I will investigate it and see which part affects this shortly

ang-zeyu · 2021-04-12T10:01:11Z

@ryoarmanda Just noticed one more edge case, but not too critical: the pages rebuilt in rebuildRequiredPages (file addition / removal) under -o mode should be adapted as well

ryoarmanda · 2021-04-13T09:44:45Z

@ryoarmanda Just noticed one more edge case, but not too critical: the pages rebuilt in rebuildRequiredPages (file addition / removal) under -o mode should be adapted as well

Ah right, need to adapt the corresponding methods as well. Thanks for the heads up! I'm working on it 👍

ryoarmanda added 2 commits March 11, 2021 02:51

Track recently viewed pages for dependency changes

3677b78

Add documentation on tracking recently viewed pages

8b53ba7

ryoarmanda changed the title ~~Track recently viewed pages for dependency changes~~ Lazy serve: Track recently viewed pages for dependency changes Mar 10, 2021

ryoarmanda added 2 commits March 12, 2021 21:47

Improve the recently viewed pages check on regenerate

601987e

Add log for recently viewed pages

4247937

Modify page generation pipeline to support sequential generation

2b1523a

Add countermeasure for live reload spam on recently viewed pages

2c8616a

ang-zeyu mentioned this pull request Mar 17, 2021

Implement live preview for pages in site.json #1514

Merged

10 tasks

ryoarmanda force-pushed the lazy-serve-watch-multiple-pages branch from 5aa76ac to 24addce Compare March 18, 2021 05:22

ryoarmanda force-pushed the lazy-serve-watch-multiple-pages branch 3 times, most recently from 6114ebb to 3a79423 Compare March 18, 2021 13:47

ang-zeyu reviewed Mar 18, 2021

View reviewed changes

ryoarmanda force-pushed the lazy-serve-watch-multiple-pages branch 2 times, most recently from bf05acb to 37d1f4a Compare March 18, 2021 17:16

Change strategy of determining recently viewed pages by patching live…

b026203

…-server

ryoarmanda force-pushed the lazy-serve-watch-multiple-pages branch from 37d1f4a to b026203 Compare March 18, 2021 17:26

ang-zeyu reviewed Mar 20, 2021

View reviewed changes

ryoarmanda added 2 commits March 20, 2021 18:06

Add todo on limitation of detecting tab open or close

5751dde

Remove sync and log opened pages on request finish

7eb57c3

ryoarmanda force-pushed the lazy-serve-watch-multiple-pages branch from 796c67c to 7eb57c3 Compare March 20, 2021 10:06

ang-zeyu reviewed Mar 20, 2021

View reviewed changes

Address reviews

067e535

ang-zeyu approved these changes Mar 20, 2021

View reviewed changes

ang-zeyu added this to the v3.0 milestone Mar 20, 2021

ryoarmanda changed the title ~~Lazy serve: Track recently viewed pages for dependency changes~~ Lazy serve: Track opened pages for dependency changes Mar 20, 2021

ang-zeyu merged commit 6537257 into MarkBind:master Mar 20, 2021

ang-zeyu mentioned this pull request Mar 21, 2021

Ideas for enhancing markbind serve -o #1405

Open

3 tasks

ryoarmanda mentioned this pull request Mar 28, 2021

Lazy serve: fix untracked tab bug on backwards navigation #1522

Merged

10 tasks

ang-zeyu mentioned this pull request Apr 10, 2021

SSR to resolve FOUC #1534

Merged

10 tasks

ang-zeyu mentioned this pull request Apr 12, 2021

Setup e2e testing for live preview features #1544

Open

ryoarmanda mentioned this pull request Apr 22, 2021

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

Merged

10 tasks

ang-zeyu mentioned this pull request Mar 10, 2022

SSR leftovers #1815

Open

8 tasks

Lazy serve: Track opened pages for dependency changes #1513

Lazy serve: Track opened pages for dependency changes #1513

Conversation

ryoarmanda commented Mar 10, 2021 • edited

wxwxwxwx9 commented Mar 12, 2021 • edited

ryoarmanda commented Mar 12, 2021 • edited

damithc commented Mar 12, 2021

ryoarmanda commented Mar 12, 2021

ang-zeyu commented Mar 12, 2021

ryoarmanda commented Mar 12, 2021 • edited

ryoarmanda commented Mar 12, 2021

ang-zeyu commented Mar 12, 2021

damithc commented Mar 12, 2021

ang-zeyu commented Mar 13, 2021

damithc commented Mar 13, 2021

ryoarmanda commented Mar 13, 2021

ryoarmanda commented Mar 13, 2021 • edited

ang-zeyu commented Mar 14, 2021 • edited

ryoarmanda commented Mar 14, 2021 • edited

ryoarmanda commented Mar 18, 2021 • edited

ryoarmanda commented Mar 18, 2021 • edited

ang-zeyu left a comment • edited

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

ang-zeyu commented Mar 19, 2021 • edited

ryoarmanda commented Mar 19, 2021

ang-zeyu commented Mar 20, 2021

Choose a reason for hiding this comment

ryoarmanda Mar 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoarmanda commented Mar 20, 2021 • edited

ang-zeyu left a 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

ang-zeyu left a comment

Choose a reason for hiding this comment

ang-zeyu commented Mar 20, 2021

ryoarmanda commented Mar 20, 2021

ang-zeyu commented Mar 21, 2021

ryoarmanda commented Mar 22, 2021

ang-zeyu commented Apr 12, 2021

ryoarmanda commented Apr 13, 2021

ryoarmanda commented Mar 10, 2021 •

edited

wxwxwxwx9 commented Mar 12, 2021 •

edited

ryoarmanda commented Mar 12, 2021 •

edited

ryoarmanda commented Mar 12, 2021 •

edited

ryoarmanda commented Mar 13, 2021 •

edited

ang-zeyu commented Mar 14, 2021 •

edited

ryoarmanda commented Mar 14, 2021 •

edited

ryoarmanda commented Mar 18, 2021 •

edited

ryoarmanda commented Mar 18, 2021 •

edited

ang-zeyu left a comment •

edited

ang-zeyu commented Mar 19, 2021 •

edited

ryoarmanda Mar 20, 2021 •

edited

ryoarmanda commented Mar 20, 2021 •

edited