Add beforeEmbed and afterEmbed hook #1571

kevinresol · 2021-05-10T12:02:25Z

Summary

Add a "beforeEmbed", "afterEmbed" hook that will receive and return the markdown token list before and after the "embed file" mechanism takes place.

This will be useful for plugins that wish to pre-process the markdown. Currently one has to parse the markdown in beforeEach, work with the parsed token list and then re-stringify the tokens into markdown string for the next step, which is wasteful. With these hooks, one can just mess with the parsed token list and go on.

What kind of change does this PR introduce?

Feature

For any code change,

Related documentation has been updated if needed
Related tests have been updated or tests have been added

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

Tested in the following browsers:

vercel · 2021-05-10T12:02:29Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/3nHYfjUq2gqrV2eqty6vgkV5MPC7
✅ Preview: https://docsify-preview-git-fork-kevinresol-tokenized-hook-docsify-core.vercel.app

codesandbox-ci · 2021-05-10T12:04:29Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4ef235f:

Sandbox	Source
docsify-template	Configuration

kevinresol · 2021-05-10T14:26:38Z

~~I am thinking about splitting it into beforeEmbed and afterEmbed...~~

Edit: done

trusktr · 2021-08-02T00:29:13Z

Hello! This seems like a useful addition.

trusktr

If we decide we are okay to merge this an have hooks that are coupled to (require) marked, then the only thing we need to do is change or describe what "embed" means. This currently won't have any meaning to someone reading the docs.

trusktr · 2021-08-02T00:30:09Z

docs/write-a-plugin.md

@@ -18,9 +18,21 @@ window.$docsify = {
        return content;
      });

+      hook.beforeEmbed(function(tokens, next) {
+        // Invoked each time after parsing the Markdown file, but before embedding is applied to the token list.


I think we need to change the wording, or otherwise describe what "embedding" is. I don't think that currently Docsify users have any concept of "embed", and the term "embed" is currently just an internal detail.

I doubt that given there is a whole page about embedding files in the documentation: https://docsify.js.org/#/embed-files

Oh, yeah, oops. 😄

Does marked give back tokens just for the embed? Or what exactly?

"tokens" is an array of token, produced by the compile.lexer(raw) call. It is indeed a vendor-specific data structure produced by marked.

docsify/src/core/render/embed.js

Line 103 in 66303fe

let tokens = compile.lexer(raw);

Docsify's embed mechanism operates on this token list as well.

trusktr · 2021-08-02T00:31:06Z

src/core/init/lifecycle.js

@@ -5,6 +5,8 @@ export function initLifecycle(vm) {
    'init',
    'mounted',
    'beforeEach',
+    'beforeEmbed',
+    'afterEmbed',
    'afterEach',
    'doneEach',
    'ready',


We should list the order of hooks, otherwise people have to find this in the code right here.

The updates to the "Write a Plugin" page do a better job of describing when hooks are invoked. The updates aren't live yet, but they can be viewed on the develop branch preview.

trusktr · 2021-08-02T00:39:39Z

src/core/render/embed.js

+export function prerenderEmbed(
+  { compiler, raw = '', fetch, beforeEmbed },
+  done
+) {


Another thing is, this hook would depend on marked, thus is markdown-compiler-specific. What if we change renderers later?

Maybe it is good for now (performance boost) and if we change markdown renderer later then it would simply be a breaking change.

Arguably even without this hook, it would be a breaking change because output of the strings can change. But with this hook in place, it could be that a different markdown renderer does not have a concept of tokens that can be passed to a hook plugin.

Also, what if we wish to add the ability to specify markdown renderers? beforeEach and afterEach would continue to make sense, but maybe beforeEmbed would not make sense if a different markdown renderer does not pass tokens out.

So the question is: do we want to add this, and be coupled to marked? Or not?

If we decide we want this, I think the way it is implemented looks good.

trusktr · 2021-08-02T00:48:25Z

test/integration/docsify.test.js

@@ -33,6 +33,8 @@ describe('Docsify', function() {

            expect(hook.init).toBeInstanceOf(Function);
            expect(hook.beforeEach).toBeInstanceOf(Function);
+            expect(hook.beforeEmbed).toBeInstanceOf(Function);
+            expect(hook.afterEmbed).toBeInstanceOf(Function);
            expect(hook.afterEach).toBeInstanceOf(Function);
            expect(hook.doneEach).toBeInstanceOf(Function);
            expect(hook.mounted).toBeInstanceOf(Function);


Sidenote, we need better hooks tests.

Would you mind adding tests so that it shows how to use the hook and tokens (and makes sure it won't break later).

The other tests are bad (they are lacking, obviously), but moving forward we should test new features at least.

Hook tests were added in #1742 + #1770. Check out /test/e2e/plugins.test.js.

trusktr

I meant to review as "Request changes", regarding the "embed" wording.

But let's chat first about if we want to have these hooks.

jhildenbiddle

--

jhildenbiddle · 2022-03-18T18:41:02Z

docs/write-a-plugin.md

@@ -18,9 +18,21 @@ window.$docsify = {
        return content;
      });

+      hook.beforeEmbed(function(tokens, next) {


I don't think we should pass tokens produced from a markdown parser to a plugin hook. As @trusktr pointed out, this makes the plugin functionality dependent on a specific markdown library which IMHO is not a good idea.

More importantly, I don't think we should pass "markdown before it becomes HTML" in two different formats (string vs tokens) to two very similar hooks:

hook.beforeEach(function(content) { // ... return content; }); hook.beforeEmbed(function(tokens) { // ... return tokens; });

If plugins authors want to work with tokens, they can generate their own tokens by accessing Docsify's renderer directly within their plugin:

window.$docsify = { plugins: [ function(hook, vm) { hook.beforeEach(function(content) { const renderer = vm.config.markdown.renderer // Do renderer stuff... return content; }); } ] }

jhildenbiddle · 2022-03-18T18:52:37Z

test/integration/docsify.test.js

@@ -33,6 +33,8 @@ describe('Docsify', function() {

            expect(hook.init).toBeInstanceOf(Function);
            expect(hook.beforeEach).toBeInstanceOf(Function);
+            expect(hook.beforeEmbed).toBeInstanceOf(Function);
+            expect(hook.afterEmbed).toBeInstanceOf(Function);
            expect(hook.afterEach).toBeInstanceOf(Function);
            expect(hook.doneEach).toBeInstanceOf(Function);
            expect(hook.mounted).toBeInstanceOf(Function);


Hook tests were added in #1742 + #1770. Check out /test/e2e/plugins.test.js.

jhildenbiddle · 2022-03-18T18:55:14Z

src/core/init/lifecycle.js

@@ -5,6 +5,8 @@ export function initLifecycle(vm) {
    'init',
    'mounted',
    'beforeEach',
+    'beforeEmbed',
+    'afterEmbed',
    'afterEach',
    'doneEach',
    'ready',


The updates to the "Write a Plugin" page do a better job of describing when hooks are invoked. The updates aren't live yet, but they can be viewed on the develop branch preview.

jhildenbiddle

This would make for a welcome addition. See in-line comments for code change requests.

FWIW, adding new beforeEmbed and afterEmbed hooks are a good approach for adding this feature as a non-breaking change. Long term, it may be preferable (TBD) to pass all markdown (cover, content, embeds, sidebar, navbar, etc.) to a single "do stuff with markdown" hook along with meta data to indicate what type of markdown it is. That's a conversation for a later date, but I wanted to mention it in case anyone was concerned with the growing number of plugin hooks.

jhildenbiddle · 2022-03-20T14:28:47Z

@kevinresol --

Checking in. This PR kinda got lost, but if you're willing to make a few updates it would make for a nice addition to Docsify.

kevinresol · 2022-03-20T14:47:49Z

Sorry but my memory about this has long gone and I don't right now have time to pick it up again...

kevinresol added 2 commits May 10, 2021 19:55

Add tokenizedEach hook

e6e346e

Add tests and docs

4f69c60

vercel bot deployed to Preview May 10, 2021 12:02 View deployment

vercel bot deployed to Preview May 10, 2021 12:03 View deployment

kevinresol marked this pull request as draft May 10, 2021 14:25

Implement beforeEmbed and afterEmbed hooks

5436ccd

vercel bot deployed to Preview May 11, 2021 01:06 View deployment

kevinresol marked this pull request as ready for review May 11, 2021 02:32

sy-records requested a review from a team May 11, 2021 07:05

Merge branch 'develop' into tokenized-hook

5602d1e

vercel bot deployed to Preview May 11, 2021 07:06 View deployment

kevinresol changed the title ~~Add tokenizedEach hook~~ Add beforeEmbed and afterEmbed hook May 11, 2021

Koooooo-7 added the needs discussion label May 13, 2021

Merge branch 'develop' into tokenized-hook

42b915e

vercel bot deployed to Preview June 24, 2021 02:45 View deployment

Merge branch 'develop' into tokenized-hook

1ace9a1

vercel bot deployed to Preview August 2, 2021 00:29 View deployment

trusktr reviewed Aug 2, 2021

View reviewed changes

trusktr requested changes Aug 2, 2021

View reviewed changes

Merge branch 'develop' into tokenized-hook

4ef235f

vercel bot deployed to Preview August 4, 2021 04:35 View deployment

jhildenbiddle reviewed Mar 18, 2022

View reviewed changes

jhildenbiddle requested changes Mar 18, 2022

View reviewed changes

kevinresol closed this Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add beforeEmbed and afterEmbed hook #1571

Add beforeEmbed and afterEmbed hook #1571

kevinresol commented May 10, 2021 •

edited

Loading

vercel bot commented May 10, 2021 •

edited

Loading

codesandbox-ci bot commented May 10, 2021 •

edited

Loading

kevinresol commented May 10, 2021 •

edited

Loading

trusktr commented Aug 2, 2021

trusktr left a comment

trusktr Aug 2, 2021

kevinresol Aug 3, 2021

trusktr Aug 4, 2021

kevinresol Aug 4, 2021

trusktr Aug 2, 2021

jhildenbiddle Mar 18, 2022

trusktr Aug 2, 2021

trusktr Aug 2, 2021

trusktr Aug 2, 2021

trusktr Aug 4, 2021 •

edited

Loading

jhildenbiddle Mar 18, 2022

trusktr left a comment

jhildenbiddle left a comment •

edited

Loading

jhildenbiddle Mar 18, 2022

jhildenbiddle Mar 18, 2022

jhildenbiddle Mar 18, 2022

jhildenbiddle left a comment

jhildenbiddle commented Mar 20, 2022

kevinresol commented Mar 20, 2022

Add beforeEmbed and afterEmbed hook #1571

Add beforeEmbed and afterEmbed hook #1571

Conversation

kevinresol commented May 10, 2021 • edited Loading

Summary

What kind of change does this PR introduce?

For any code change,

Does this PR introduce a breaking change? (check one)

Related issue, if any:

Tested in the following browsers:

vercel bot commented May 10, 2021 • edited Loading

codesandbox-ci bot commented May 10, 2021 • edited Loading

kevinresol commented May 10, 2021 • edited Loading

trusktr commented Aug 2, 2021

trusktr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr left a comment

Choose a reason for hiding this comment

jhildenbiddle left a comment • edited Loading

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

jhildenbiddle left a comment

Choose a reason for hiding this comment

jhildenbiddle commented Mar 20, 2022

kevinresol commented Mar 20, 2022

kevinresol commented May 10, 2021 •

edited

Loading

vercel bot commented May 10, 2021 •

edited

Loading

codesandbox-ci bot commented May 10, 2021 •

edited

Loading

kevinresol commented May 10, 2021 •

edited

Loading

trusktr Aug 4, 2021 •

edited

Loading

jhildenbiddle left a comment •

edited

Loading