Fix plugin live reloading for relative file paths #974

ang-zeyu · 2020-01-10T16:14:10Z

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

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

Fixes #913

What is the rationale for this request?
As mentioned in #913, relative file paths for sources defined by plugins are not correctly resolved
against the page being processed. If the source is also part of an included file, then it should be resolved against the included file.

Urls are also added to the page's pluginSourceFiles, which has no effect, but is incorrect

What changes did you make? (Give an overview)
Let's fix this and simplify the api for defining sources.

getSources now returns

{
  tagMap: [ ['tag name', 'src attribute name' ], ['tag name', 'src attribute name' ], ... ]
  sources: as before, which is an array of absolute / relative file paths to watch
}

From a plugin author's perspective, resolving said sources against the page being processed
/ the included file is tricky. This moves all such logic to Page.js, exposing a simpler api.

The old return syntax is deprecated, but still supported as well.

Provide some example code that this change will affect:

getSources: () => ({
  tagMap: [['puml', 'src']],
}),

Testing instructions:
Simply use npm run test to run the new unit tests,

For manual testing of live reloading, the test_site has some
sample pumls at the bottom.

Proposed commit message: (wrap lines at 72 characters)
Fix plugin live reloading for relative file paths

Relative file paths provided by plugins are not resolved against the
processed page or included file.

This results in live reloading failing for pages not in the root
directory.

Let's fix this, and also rewrite plugin source collection, allowing
plugins to simply define tags and corresponding source attributes
to watch.

yamgent

Tried it out and seems to be working, nice job. 👍

yamgent · 2020-01-12T12:54:22Z

test/unit/Page.test.js

 const Page = require('../../src/Page');

+


Remove the extra blank line

yamgent · 2020-01-12T13:09:27Z

docs/userGuide/usingPlugins.md

+- **Parameters:**
+    - `content`: The raw Markdown of the current Markdown file (`.md`, `.mbd`, etc.).
+    - `pluginContext`: User provided parameters for the plugin. This can be specified in the `site.json`.
+    - `frontMatter`: The frontMatter of the page being processed, in case any frontMatter data is required.


Any reason for deviating from the original ordering that the others follow (i.e. parameters, then what it returns)? Seems like the existing format can still be used here, even if it has more information compared to the rest like getLinks() and getScripts().

yamgent · 2020-01-12T13:15:47Z

test/unit/Page.test.js

+    path.resolve('/root/subdir/images/sample2.png'))).toEqual(true);
+  expect(page.pluginSourceFiles.has(
+    path.resolve('/root/images/sample3.png'))).toEqual(true);
+  expect(page.pluginSourceFiles.has(


Seems that the pluginSourceFiles variable should just be a Set.

test/unit/utils/pageData.js

ang-zeyu · 2020-01-13T16:03:01Z

Updated, thanks for looking through it!

yamgent

Ok, now just nits in documentations.

docs/userGuide/usingPlugins.md

yamgent · 2020-01-14T01:53:25Z

docs/userGuide/usingPlugins.md

+  - For relative file paths, _if the tag is part of some included content_ ( eg. `<include />` tags ), it will be resolved against the included page.
+    Otherwise, it is resolved against the page being processed.
+- `sources`: An array of source file paths to watch, where relative file paths are resolved only against the page being processed.
+- You can also directly return an array of source file paths to watch ( ie. the `sources` field ) ___(deprecated)___


In fact the use case here and the one above seems to be the same, do we need this line in that case?

Hmm, I actually meant only to deprecate the old syntax, not the functionality of returning some manually specified file paths (where relative ones are resolved according to the file processed).

This can be useful if a plugin needs live reload for a file that isn't specified as some src attribute in the dom. E.g. a plugin where you can have file_name+_page_extension that adds some dynamic content to the relavant page based on that file's contents.

Hmm ok, the sentence standalone feels a bit difficult to comprehend without your additional explanation (because of the (ie. the sources field) portion), but I am fine with leaving it as it is for the moment.

yamgent · 2020-01-14T01:53:47Z

docs/userGuide/usingPlugins.md

-  - `pluginContext`: User provided parameters for the plugin. This can be specified in the `site.json`.
-  - `frontMatter`: The frontMatter of the page being processed, in case any frontMatter data is required.
+
+`getSources(content, pluginContext, frontMatter)`: called _before_ a Markdown file's `preRender` function is called.


Minor nit: called -> Called

yamgent · 2020-01-14T01:55:23Z

docs/userGuide/usingPlugins.md

+- `tagMap`: An array consisting of `['tag name', 'source attribute name']` key value pairs.
+  - MarkBind will automatically search for matching tags with the source attributes, and watch them.
+  - For relative file paths, _if the tag is part of some included content_ ( eg. `<include />` tags ), it will be resolved against the included page.
+    Otherwise, it is resolved against the page being processed.


Minor nit: join this line with the previous line?

yamgent · 2020-01-14T02:00:33Z

docs/userGuide/usingPlugins.md

+  - For relative file paths, _if the tag is part of some included content_ ( eg. `<include />` tags ), it will be resolved against the included page.
+    Otherwise, it is resolved against the page being processed.
+- `sources`: An array of source file paths to watch, where relative file paths are resolved only against the page being processed.
+- You can also directly return an array of source file paths to watch ( ie. the `sources` field ) ___(deprecated)___

 Example usage of `getSources` from the PlantUML plugin:


I feel that the reader might have a bit of difficulty understanding the example without context. Maybe an addition like this will make it clearer?

"Example usage of getSources from the PlantUML plugin, which will watch files from tags such as <puml src="..." />:"

Relative file paths provided by plugins are not resolved against the processed page or included file. This results in live reloading failing for pages not in the root directory. Let's fix this, and also rewrite plugin source collection, allowing plugins to simply define tags and corresponding source attributes to watch. This moves the logic for checking whether the source is part of an included file to the main markbind repo, reducing potential code duplication and simplifying the api.

ang-zeyu · 2020-01-14T11:21:25Z

Updated, save for the deprecated syntax parts

yamgent reviewed Jan 12, 2020

View reviewed changes

ang-zeyu force-pushed the plugin-live-reload-fix branch from ace11a2 to 43d6f5e Compare January 13, 2020 16:01

yamgent reviewed Jan 14, 2020

View reviewed changes

ang-zeyu force-pushed the plugin-live-reload-fix branch from 43d6f5e to 6ab3341 Compare January 14, 2020 11:18

yamgent approved these changes Jan 15, 2020

View reviewed changes

yamgent added this to the v2.9.1 milestone Jan 15, 2020

yamgent merged commit 0acd97c into MarkBind:master Jan 16, 2020

le0tan mentioned this pull request Jan 22, 2020

Adopt ES6 class syntax #979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix plugin live reloading for relative file paths #974

Fix plugin live reloading for relative file paths #974

ang-zeyu commented Jan 10, 2020

yamgent left a comment

yamgent Jan 12, 2020

yamgent Jan 12, 2020

yamgent Jan 12, 2020

ang-zeyu commented Jan 13, 2020

yamgent left a comment

yamgent Jan 14, 2020

ang-zeyu Jan 14, 2020

yamgent Jan 15, 2020

yamgent Jan 14, 2020

yamgent Jan 14, 2020

yamgent Jan 14, 2020

ang-zeyu commented Jan 14, 2020

Fix plugin live reloading for relative file paths #974

Fix plugin live reloading for relative file paths #974

Conversation

ang-zeyu commented Jan 10, 2020

yamgent 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

ang-zeyu commented Jan 13, 2020

yamgent 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

Choose a reason for hiding this comment

ang-zeyu commented Jan 14, 2020