-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add plugin local asset collection #1129
Conversation
3a1202e
to
a4f55ac
Compare
cd2227a
to
a17c456
Compare
src/Page.js
Outdated
@@ -983,6 +984,28 @@ class Page { | |||
* Collect page content inserted by plugins | |||
*/ | |||
collectPluginsAssets(content) { | |||
const getResolvedAssetElement = (html, tagName, attrName, plugin, pluginName) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only one suggestion:
The implementation of utils.buildScript
and buildStyleSheet
plus this getResolvedElement
seems rather redundant, where build* produces <script src =...></script>
, and getResolvedElement later reparses it. If buildScript could instead return something like
{ type: 'scriptsrc', src: 'file.js' }
then we wouldn't need to use cheerio at all, just convert the src first and then return <script src =convertedsrc.js></script>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One con with this suggestion would be that the user cannot manually return <script src = path></script>
.
Maybe it's better to leave it in so the user has more choices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked the buildScript/buildStylesheet
a little with the new pluginUtils
from #1100, its the same from a plugin author's standpoint though ( changes are limited to the first of the 4 commits ).
{ type: 'scriptsrc', src: 'file.js' }
for this, I think returning <script/link>
directly is more intuitive from a plugin standpoint, since we represent html in html and not in json. This way the author can also add other variables to the tag as well easily, e.g. <link prefetch ... />
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but now the user has to use buildScript if they want an external link.
So they can't add other attributes like <link prefetch href = "externa.css" />
.
The first implementation is probably better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, reverted that. ( getResolvedAssetElement
is able to stay in pluginUtils
though )
a17c456
to
0072df5
Compare
Nice refactor! LGTM, only one small suggestion. |
a6f73ab
to
a7420c0
Compare
a7420c0
to
093b59f
Compare
@marvinchin could I get your thoughts here as well? since you opened up #839 =) The main change here being letting plugins add local assets instead of just ones from external cdns ( first of the 4 commits ) |
Sorry for the delays 🙏 I'll look at this tomorrow! |
No rush, not in a hurry to get this merged! 😄 |
093b59f
to
1a15f4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase your PR 🙂
1a15f4c
to
735b706
Compare
735b706
to
ff4a5b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation mostly looks good to me, just a couple of comments.
src/Page.js
Outdated
this.frontMatter, linkUtils); | ||
let pluginLinks = plugin.getLinks(content, this.pluginsContext[pluginName], | ||
this.frontMatter, linkUtils); | ||
pluginLinks = pluginLinks.map(linkHtml => getResolvedAssetElement(linkHtml, this.baseUrl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep pluginLinks
and const
and delcare a new const resolvedPluginLinks
instead?
src/util/pluginUtil.js
Outdated
@@ -50,4 +53,42 @@ const pluginUtil = { | |||
}, | |||
}; | |||
|
|||
module.exports = pluginUtil; | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intention of pluginUtil
is to contain a set of useful functions for plugin writers to use. getResolvedAssetElement
seems to be something more internal to how markbind handles assets from plugins, and does not need to be exposed to plugin writers. So maybe it should belong somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it back to page
@@ -13,63 +13,47 @@ function scrollToUrlAnchorHeading() { | |||
} | |||
} | |||
|
|||
function flattenModals() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm - this isn't needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, its a leftover from the bootstrap-vue PR
src/Site.js
Outdated
this.plugins[plugin]._pluginAssetOutputPath = path.resolve(this.outputPath, | ||
PLUGIN_SITE_ASSET_FOLDER_NAME, plugin); | ||
|
||
fs.mkdirSync(this.plugins[plugin]._pluginAssetOutputPath, { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if plugins have assets, the sources might be urls right? In that case, there might not be any assets copied to the output folder. Should we defer the creation of the folder until the point that we are copying it over?
Also just to note, this PR could have been split into two parts - one adding support for local assets, and then the other one using it to fix |
ff4a5b4
to
e8094a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
@nbriannl need you to remove the change request =X updated below on latest, no changes |
dfab13c
to
1fadfbb
Compare
Plugins might want to package their own assets for use instead of relying on external sources only. Let's add this, allowing the getLinks and getScripts methods to return link and script elements that have a relative or absolute file path as its src or href attributes.
The main markbind code has some amount of logic and styles relating to the anchor plugin. Let's encapsulate the anchor plugin's logic and styles within the plugin itself, which should lead to better code readability. This also allows the user to turn off the plugin without risk of side effects.
1fadfbb
to
f672dcf
Compare
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 #839
#442 - fixed with
stopPropagation
instead of relying on runtimewindow.location.href
shifting#433's hiding/displaying of anchor icons - done using css instead
What is the rationale for this request?
There is some degree of logic within the markbind core code that is related to plugins.
This is because the plugin
getLinks
andgetScripts
methods only allow linking to external sources.We can enhance the API hence, allowing us to encapsulate plugin logic within itself, which should lead to better code readability.
What changes did you make? (Give an overview)
Commit organization:
markbind-plugin-anchors
using this new featureIs there anything you'd like reviewers to focus on?
Was there a reason #433 was done using runtime jQuery?
Testing instructions:
npm run test
should passProposed commit message: (wrap lines at 72 characters)
Add local asset collection for plugins
Plugins might want to package their own assets for use instead of
relying on external sources only.
The main markbind code also has some amount of logic and styles
relating to the anchor plugin.
Let's add this, allowing the getLinks and getScripts methods to return
link and script elements that have a relative or absolute file path as
its src or href attributes.
Let's encapsulate the anchor plugin's logic and styles within the
plugin itself, which should lead to better code readability.
This also allows the user to turn off the plugin without risk of side
effects.