Fix lazy reload for urls without index #1110

ang-zeyu · 2020-03-09T15:40:06Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Bug fix

What is the rationale for this request?
To normalize url requests that reference index.html implcitly ( eg hostname/userGuide, hostname/userGuide/ ), such that a page build is triggered ( if needed ).

What changes did you make? (Give an overview)
Added some code to normalize urls that reference index.html implicitly

Provide some example code that this change will affect:

// Maps hostname/userGuide/  to hostname/userGuide/index
normalizedUrl = (hasNoExtension || hasEndingSlash)
  ? path.posix.join(normalizedUrl, 'index')
  : normalizedUrl;

Is there anything you'd like reviewers to focus on?
na

Testing instructions:
lazy reload for urls that reference index.html implicitly ( eg. userGuide/, userGuide ) should now correctly trigger a page build ( if necessary )

Proposed commit message: (wrap lines at 72 characters)
Fix lazy reload for urls without index

Lazy live reloading uses the file extension of the request url to
determine whether the request is for a html page, and not from a
dynamic include.
Hence, requests for index.html with urls that do not end with
index.html fail to be detected.

Let’s add extra processing to detect such requests.

ang-zeyu · 2020-03-21T14:04:10Z

@nbriannl could I get your review here?

Aim of the changes here being to normalize host/userGuide/ or host/userGuide ( if host/userGuide is not a raw file ) to host/userGuide/index ( intentionally extensionless )

nbriannl · 2020-03-22T08:40:05Z

If i understand correctly,
With this change, when markbind serve and change docs\userGuide\index.md, I will trigger a live reload on both http://127.0.0.1:8080/userGuide/ and http://127.0.0.1:8080/userGuide/index.html

Before this change, only http://127.0.0.1:8080/userGuide/index.html, would live reload?

ang-zeyu · 2020-03-22T08:43:21Z

If i understand correctly,
With this change, when markbind serve and change docs\userGuide\index.md, I will trigger a live reload on both http://127.0.0.1:8080/userGuide/ and http://127.0.0.1:8080/userGuide/index.html

Before this change, only http://127.0.0.1:8080/userGuide/index.html, would live reload?

Yup

nbriannl

Looks good, works when I tested. But I think someone more experienced with this should approve

marvinchin

Thanks for looking at this :) just a suggestion from me

marvinchin · 2020-03-25T16:15:53Z

index.js

+              return;
+            }
+
+            let normalizedUrl = req.url.replace(config.baseUrl, '');


We don't use normalizedUrl until L208. Could we define normalizedUrl closer to it's usage?

Also, I generally prefer using const and naming intermediate variables explicitly. I find that this makes it easier to follow what each step is doing.

Good catch 😅, thanks! Updated some of the comments as well

marvinchin · 2020-03-26T15:42:24Z

index.js

+            let normalizedUrl = req.url.replace(config.baseUrl, '');
+
+            // Map 'hostname/userGuide/' and 'hostname/userGuide' to hostname/userGuide/index.
+            normalizedUrl = (hasNoExtension || hasEndingSlash)


Should we do:

const urlWithoutBase = ... const urlWithIndex = ... const urlWithoutExtension = ...

I think we should avoid mutation where possible. In this case, we can break this transformation down into individual independent steps with variables names that make each step self-explanatory.

The logical flow is fairly straightforward here, but in other more complex cases variable mutation can lead to some hard to trace errors. I have a strong preference for const over let whenever possible to avoid this, but I'm happy to discuss if you (or anyone else) has a different preference :)

marvinchin · 2020-03-26T15:45:05Z

index.js

@@ -179,21 +179,42 @@ program

        if (onePagePath) {
          const lazyReloadMiddleware = function (req, res, next) {
-            const isHtmlFile = req.url.endsWith('.html');
+            const requestExtension = path.posix.extname(req.url);


I think just extension or urlExtension is fine? 😛 req itself is an object that has a bunch of other stuff in it, so requestExtension doesn't feel very correct to me.

Lazy live reloading uses the file extension of the request url to determine whether the request is for a html page, and not from a dynamic include. Hence, requests for index.html with urls that do not end with index.html fail to be detected. Let’s add extra processing to detect such requests.

ang-zeyu · 2020-03-26T15:58:08Z

Updated the variable naming

marvinchin

LGTM :)

* 'master' of https://github.com/MarkBind/markbind: Fix lazy reload for urls without index (MarkBind#1110) Changes remaining references from filterTags to tags (MarkBind#1149)

…bind into remove-fixed-bugs * 'remove-fixed-bugs' of https://github.com/Tejas2805/markbind: Docs: Add contributing.md (MarkBind#1139) Show pointer and use underline dashed for click trigger (MarkBind#1111) Support variables to be defined as by JSON (MarkBind#1117) Allow an array for specifying page src or glob (MarkBind#1118) Code blocks: Implement inline markdown support for heading (MarkBind#1137) Fix lazy reload for urls without index (MarkBind#1110) Changes remaining references from filterTags to tags (MarkBind#1149)

Lazy live reloading uses the file extension of the request url to determine whether the request is for a html page, and not from a dynamic include. Hence, requests for index.html with urls that do not end with index.html fail to be detected. Let’s add extra processing to detect such requests.

ang-zeyu force-pushed the fix-lazy-reload-no-index-requests branch from 329c861 to 411b33c Compare March 9, 2020 15:41

ang-zeyu requested a review from nbriannl March 15, 2020 10:54

ang-zeyu force-pushed the fix-lazy-reload-no-index-requests branch 2 times, most recently from cc1062a to 3c106d8 Compare March 21, 2020 13:58

ang-zeyu force-pushed the fix-lazy-reload-no-index-requests branch from 3c106d8 to 5a69c47 Compare March 22, 2020 06:44

nbriannl reviewed Mar 22, 2020

View reviewed changes

ang-zeyu force-pushed the fix-lazy-reload-no-index-requests branch from 5a69c47 to f041c7e Compare March 23, 2020 09:54

ang-zeyu requested a review from marvinchin March 25, 2020 06:14

marvinchin requested changes Mar 25, 2020

View reviewed changes

ang-zeyu force-pushed the fix-lazy-reload-no-index-requests branch from f041c7e to bd21d45 Compare March 26, 2020 12:51

ang-zeyu requested a review from marvinchin March 26, 2020 12:52

marvinchin reviewed Mar 26, 2020

View reviewed changes

ang-zeyu force-pushed the fix-lazy-reload-no-index-requests branch from bd21d45 to b33c554 Compare March 26, 2020 15:56

marvinchin approved these changes Mar 26, 2020

View reviewed changes

marvinchin added this to the v2.12.1 milestone Mar 26, 2020

marvinchin merged commit d3a07aa into MarkBind:master Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lazy reload for urls without index #1110

Fix lazy reload for urls without index #1110

ang-zeyu commented Mar 9, 2020

ang-zeyu commented Mar 21, 2020

nbriannl commented Mar 22, 2020 •

edited

Loading

ang-zeyu commented Mar 22, 2020

nbriannl left a comment

marvinchin left a comment

marvinchin Mar 25, 2020

ang-zeyu Mar 26, 2020

marvinchin Mar 26, 2020

marvinchin Mar 26, 2020

marvinchin Mar 26, 2020

ang-zeyu commented Mar 26, 2020

marvinchin left a comment

Fix lazy reload for urls without index #1110

Fix lazy reload for urls without index #1110

Conversation

ang-zeyu commented Mar 9, 2020

ang-zeyu commented Mar 21, 2020

nbriannl commented Mar 22, 2020 • edited Loading

ang-zeyu commented Mar 22, 2020

nbriannl left a comment

Choose a reason for hiding this comment

marvinchin left a comment

Choose a reason for hiding this comment

marvinchin Mar 25, 2020

Choose a reason for hiding this comment

ang-zeyu Mar 26, 2020

Choose a reason for hiding this comment

marvinchin Mar 26, 2020

Choose a reason for hiding this comment

marvinchin Mar 26, 2020

Choose a reason for hiding this comment

marvinchin Mar 26, 2020

Choose a reason for hiding this comment

ang-zeyu commented Mar 26, 2020

marvinchin left a comment

Choose a reason for hiding this comment

nbriannl commented Mar 22, 2020 •

edited

Loading