Allow live reloading for plugin defined sources #955

ang-zeyu · 2019-12-15T13:42:53Z

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Resolves #913

What is the rationale for this request?
To allow plugins to define their own source file types and corresponding
source files that will be watched and updated by the live preview appropriately.

What changes did you make? (Give an overview)

Plugins can a new attribute in its exports: getSources
- getSources: Allows plugins to return an array of source file paths to watch
Similar to Page.prototype.preRender, Page.prototype.getSources runs getSources all plugins just before
preRender.
Updated relavant test files

Provide some example code that this change will affect:

(Plant uml plugin file)
module.exports = {
  ...,
  getSources: (content) => {
    // Add all src attributes in <puml> tags to watch list
    const $ = cheerio.load(content, { xmlMode: true });

    return $('puml').map((i, tag) => tag.attribs.src).get();
  },

Is there anything you'd like reviewers to focus on?
Whether a callback style solution would be better ( a 5th parameter to preRender / postRender that contains callbacks plugins can use ), since in the long run one may find plugins requiring more and more functionality.

Testing instructions:
I added a <puml> tag, and an <include> tag ( including testPlantUML.md )
to the test site. Changing any of the source files ( e.g. activity.puml ) should
cause the affected pages to reload when serving the page.

Proposed commit message: (wrap lines at 72 characters)
Allow live reloading for plugin defined sources

This allows plugins to define a custom list of source files and file
types for the page to be watched, allowing live reload to rebuild
the affected page.

acjh · 2019-12-15T14:00:01Z

src/Page.js

+    if (plugin.getSources) {
+      const sources = _.castArray(
+        plugin.getSources(content, this.pluginsContext[pluginName] || {},
+                          this.frontMatter, this.getPluginConfig()));


Why _.castArray? getSources should return an array.

So that a single source file could be returned as well,
should I enforce returning an array instead?

Yes, since getSources is plural.

ang-zeyu · 2019-12-15T14:47:53Z

Updated getSources to require returning array instead as per method naming

yamgent

Seems good implementation-wise, comments are mostly about wordings:

yamgent · 2019-12-17T02:41:56Z

docs/userGuide/usingPlugins.md

+      const $ = cheerio.load(content, { xmlMode: true });
+
+      return $('puml').map((i, tag) => tag.attribs.src).get();
+    },


The indentation of the method body is off. Only the comment line was in the correct indentation

yamgent · 2019-12-17T02:42:17Z

docs/userGuide/usingPlugins.md

+During the `preRender` and `postRender` stages however, plugins may do custom processing using some other
+source file types, as parsed from the raw Markdown, typically requiring rebuilding the site.
+
+Hence, to add custom source files to watch, MarkBind allows defining an additional method in the plugin.


Hence, to add custom source files to watch, ~~MarkBind allows defining an additional method in the plugin.~~ implement the ``getSources`` method:

Prefer being more direct in documentations so that it is easier for the reader to process.

yamgent · 2019-12-17T02:45:49Z

docs/userGuide/usingPlugins.md

+
+Hence, to add custom source files to watch, MarkBind allows defining an additional method in the plugin.
+- `getSources(content, pluginContext, frontMatter, config)`: Called before the `preRender` function to retrieve an array of source file paths to watch.
+  - `content`: The raw Markdown of any Markdown file (`.md`, `.mbd`, etc.).


any -> the current? The use of "any" makes it seems like this call is not tied to any pages.

yamgent · 2019-12-17T02:51:10Z

docs/userGuide/usingPlugins.md

+source file types, as parsed from the raw Markdown, typically requiring rebuilding the site.
+
+Hence, to add custom source files to watch, MarkBind allows defining an additional method in the plugin.
+- `getSources(content, pluginContext, frontMatter, config)`: Called before the `preRender` function to retrieve an array of source file paths to watch.


Returns an array of source file paths to watch. Called before a Markdown file'spreRender function is called.

yamgent · 2019-12-17T02:56:26Z

src/Page.js

 };

+/**
+ * Collect page sources provided by plugins


Seems a bit wrong to say "page sources", since puml files are not really pages? Maybe "file sources"?

ang-zeyu · 2019-12-17T05:31:24Z

Hi, thanks for looking through it! I've updated the relavant lines.

On another note, I noticed that usingPlugins.md does not have documentation for the fourth parameter, config, even though it is used in the preRender and postRender functions. I've correspondingly also omitted it from getSources for now.

Was this an intentional decision to hide some of the implementation details? If not, I could update it as well.

yamgent · 2019-12-18T03:02:18Z

Was this an intentional decision to hide some of the implementation details? If not, I could update it as well.

It was probably an oversight. Feel free to add it in.

ang-zeyu · 2019-12-18T08:24:13Z

Updated, will squash if its fine!

acjh · 2019-12-18T16:55:03Z

I noticed that usingPlugins.md does not have documentation for the fourth parameter, config, even though it is used in the preRender and postRender functions. I've correspondingly also omitted it from getSources for now.

Was this an intentional decision to hide some of the implementation details? If not, I could update it as well.

It was probably an oversight. Feel free to add it in.

Actually, it was intentionally undocumented by @jamos-tay as of #714 (comment):

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful

I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation

ang-zeyu · 2019-12-18T17:46:36Z

Actually, it was intentionally undocumented by @jamos-tay as of [#714 (comment)]

Thanks for the heads up!

Since @jamos-tay ended up going with the approach @marvinchin suggested, there isn't too much to document though. Regarding the original concern for the change ( passing the entire Page being dangerous ), I could include a warning in the docs about includedFiles since it is used in Site.proto.updateSiteData(). Still, its a subsection to add into the docs which means more details for the user to read. Other properties aside though, rootPath, sourcePath and resultPath seem to be quite useful for plugins.

What's the take on this? 😮

acjh · 2019-12-19T01:45:12Z

Since @jamos-tay ended up going with the approach @marvinchin suggested, there isn't too much to document though. ... Other properties aside though, rootPath, sourcePath and resultPath seem to be quite useful for plugins.

The concern isn't that it is tedious to document. Rather, ideally, plugins should purely process content rather than use variables from and depend on the (filesystem-aware) implementation of Markbind CLI.

ang-zeyu · 2019-12-19T07:15:07Z

Noted on the intended usage of plugins. Thanks!

Reverted the 'documentation updates' and squashed

acjh reviewed Dec 15, 2019

View reviewed changes

ang-zeyu force-pushed the watch-puml branch 2 times, most recently from 5dd6389 to 720648b Compare December 15, 2019 14:47

yamgent self-requested a review December 16, 2019 03:50

ang-zeyu force-pushed the watch-puml branch from 720648b to 945d040 Compare December 16, 2019 19:28

ang-zeyu changed the title ~~[WIP] Allow live reloading for plugin defined sources~~ Allow live reloading for plugin defined sources Dec 16, 2019

ang-zeyu force-pushed the watch-puml branch from ddeeca9 to 7e740eb Compare December 16, 2019 20:14

yamgent requested changes Dec 17, 2019

View reviewed changes

ang-zeyu force-pushed the watch-puml branch from 7e740eb to 2943cab Compare December 17, 2019 05:28

ang-zeyu force-pushed the watch-puml branch from 2a8b519 to ae6c019 Compare December 18, 2019 08:23

Allow live reloading for plugin defined sources

c8f6227

ang-zeyu force-pushed the watch-puml branch from ae6c019 to c8f6227 Compare December 19, 2019 07:12

yamgent approved these changes Dec 20, 2019

View reviewed changes

yamgent merged commit 211cd6e into MarkBind:master Dec 23, 2019

yamgent added this to the v2.7.1 milestone Dec 23, 2019

Allow live reloading for plugin defined sources #955

Allow live reloading for plugin defined sources #955

Uh oh!

Conversation

ang-zeyu commented Dec 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Dec 15, 2019

Uh oh!

yamgent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Dec 17, 2019

Uh oh!

yamgent commented Dec 18, 2019

Uh oh!

ang-zeyu commented Dec 18, 2019

Uh oh!

acjh commented Dec 18, 2019

Uh oh!

ang-zeyu commented Dec 18, 2019

Uh oh!

acjh commented Dec 19, 2019

Uh oh!

ang-zeyu commented Dec 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ang-zeyu commented Dec 15, 2019 •

edited

Loading