Skip to content

Fix live server issue with lazy loading from CLI refactor#2266

Merged
lhw-1 merged 1 commit intoMarkBind:masterfrom
lhw-1:0-missing-res
Apr 6, 2023
Merged

Fix live server issue with lazy loading from CLI refactor#2266
lhw-1 merged 1 commit intoMarkBind:masterfrom
lhw-1:0-missing-res

Conversation

@lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Apr 6, 2023

What is the purpose of this pull request?

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

Overview of changes:

Adds the res parameter to the lazyReloadMiddleware function that was omitted during the refactor in #2239.

While this parameter was accidentally removed due to not being used within the function itself, the res property is needed for functions passed into the live server (for lazy preview).

Thanks @yucheng11122017 for pointing it out!

Anything you'd like to highlight/discuss:

I've done a sanity check once more to make sure that there were no unnecessary changes made to the functions. Here are the list of changes to the functions:

  • syncOpenedPages has been modified to take in the site parameter instead of none (i.e. () => ... to (site => ...). This is to allow it access to the site variable, which it could previously access as it was in the serve command declaration. The rest of the function was NOT modified.
  • addHandler, changeHandler, removeHandler, and lazyReloadMiddleware functions were previously passed in as functions to other functions. They now take an additional set of parameters that return the function itself, done to allow it access to variables in the same manner as syncOpenedPages (i.e. (filePath) => ... to (site, onePagePath) => (filePath) => ...). The returned functions themselves were NOT modified (aside from lazyReloadMiddleware.

lazyReloadMiddleware was the only exception, as the parameter res was flagged as unused and was erroneously removed. This PR adds it back.

The bug was not flagged by the tests as there are currently no pre-existing unit tests for the markbind serve command. Unit tests should be implemented for functions in serveUtils, which may have to modify these functions to be tested as needed. Additionally, more functional tests for serve should be considered, though they are not trivial to implement.

Testing instructions:

Try to serve with markbind serve -d -o FILENAME.

Originally: Displays an error saying "next is not a function".

New: Renders correctly.

Proposed commit message: (wrap lines at 72 characters)
Fix live server issue with lazy loading from CLI refactor


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@yucheng11122017
Copy link
Contributor

Might be good to add testing instructions for this PR!
Try to serve with markbind serve -d -o FILENAME

Originally: next is not a function

New: Renders correctly

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @lhw-1

@lhw-1 lhw-1 merged commit bee3473 into MarkBind:master Apr 6, 2023
lhw-1 added a commit that referenced this pull request Apr 6, 2023
tlylt pushed a commit to tlylt/markbind that referenced this pull request Apr 6, 2023
@lhw-1 lhw-1 changed the title Add res parameter back to lazyReloadMiddleware Fix live server issue with lazy loading from CLI refactor Apr 9, 2023
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.

2 participants