Skip to content
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.

Sign up for GitHub

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 beforeEmbed and afterEmbed hook #1571

Closed
wants to merge 7 commits into from
Closed

Add beforeEmbed and afterEmbed hook #1571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6e346e
Add tokenizedEach hook
kevinresol May 10, 2021
4f69c60
Add tests and docs
kevinresol May 10, 2021
5436ccd
Implement beforeEmbed and afterEmbed hooks
kevinresol May 11, 2021
5602d1e
Merge branch 'develop' into tokenized-hook
sy-records May 11, 2021
42b915e
Merge branch 'develop' into tokenized-hook
Koooooo-7 Jun 24, 2021
1ace9a1
Merge branch 'develop' into tokenized-hook
trusktr Aug 2, 2021
4ef235f
Merge branch 'develop' into tokenized-hook
sy-records Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions docs/write-a-plugin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,21 @@ window.$docsify = {
return content;
});

hook.beforeEmbed(function(tokens, next) {
Copy link
Member

Choose a reason for hiding this comment

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

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;
      });
    }
  ]
}

// Invoked each time after parsing the Markdown file, but before embedding is applied to the token list.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah, oops. 😄

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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.

// ...
next(tokens);
});

hook.afterEmbed(function(tokens, next) {
// Invoked each time after parsing the Markdown file, and after embedding is applied to the token list.
// ...
next(tokens);
});

hook.afterEach(function(html, next) {
// Invoked each time after the Markdown file is parsed.
// beforeEach and afterEach support asynchronous。
// Invoked each time after the Markdown file is parsed and the result HTML is generated.
// beforeEach, beforeEmbed, afterEmbed and afterEach support asynchronous。
// ...
// call `next(html)` when task is done.
next(html);
Expand Down
2 changes: 2 additions & 0 deletions src/core/init/lifecycle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ export function initLifecycle(vm) {
'init',
'mounted',
'beforeEach',
'beforeEmbed',
'afterEmbed',
'afterEach',
'doneEach',
'ready',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Expand Down
104 changes: 56 additions & 48 deletions src/core/render/embed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ function walkFetchEmbed({ embedTokens, compile, fetch }, cb) {
}
}

export function prerenderEmbed({ compiler, raw = '', fetch }, done) {
export function prerenderEmbed(
{ compiler, raw = '', fetch, beforeEmbed },
done
) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

let hit = cached[raw];
if (hit) {
const copy = hit.slice();
Expand All @@ -100,56 +103,61 @@ export function prerenderEmbed({ compiler, raw = '', fetch }, done) {
}

const compile = compiler._marked;
let tokens = compile.lexer(raw);
const embedTokens = [];
const linkRE = compile.Lexer.rules.inline.link;
const links = tokens.links;

tokens.forEach((token, index) => {
if (token.type === 'paragraph') {
token.text = token.text.replace(
new RegExp(linkRE.source, 'g'),
(src, filename, href, title) => {
const embed = compiler.compileEmbed(href, title);

if (embed) {
embedTokens.push({
index,
embed,
});
}

return src;
}
);
}
});
if (!beforeEmbed) {
beforeEmbed = (tokens, done) => done(tokens);
} // noop
beforeEmbed(compile.lexer(raw), tokens => {
const embedTokens = [];
const linkRE = compile.Lexer.rules.inline.link;
const links = tokens.links;

tokens.forEach((token, index) => {
if (token.type === 'paragraph') {
token.text = token.text.replace(
new RegExp(linkRE.source, 'g'),
(src, filename, href, title) => {
const embed = compiler.compileEmbed(href, title);

if (embed) {
embedTokens.push({
index,
embed,
});
}

// keep track of which tokens have been embedded so far
// so that we know where to insert the embedded tokens as they
// are returned
const moves = [];
walkFetchEmbed({ compile, embedTokens, fetch }, ({ embedToken, token }) => {
if (token) {
// iterate through the array of previously inserted tokens
// to determine where the current embedded tokens should be inserted
let index = token.index;
moves.forEach(pos => {
if (index > pos.start) {
index += pos.length;
}
});
return src;
}
);
}
});

// keep track of which tokens have been embedded so far
// so that we know where to insert the embedded tokens as they
// are returned
const moves = [];
walkFetchEmbed({ compile, embedTokens, fetch }, ({ embedToken, token }) => {
if (token) {
// iterate through the array of previously inserted tokens
// to determine where the current embedded tokens should be inserted
let index = token.index;
moves.forEach(pos => {
if (index > pos.start) {
index += pos.length;
}
});

merge(links, embedToken.links);
merge(links, embedToken.links);

tokens = tokens
.slice(0, index)
.concat(embedToken, tokens.slice(index + 1));
moves.push({ start: index, length: embedToken.length - 1 });
} else {
cached[raw] = tokens.concat();
tokens.links = cached[raw].links = links;
done(tokens);
}
tokens = tokens
.slice(0, index)
.concat(embedToken, tokens.slice(index + 1));
moves.push({ start: index, length: embedToken.length - 1 });
} else {
cached[raw] = tokens.concat();
tokens.links = cached[raw].links = links;
done(tokens);
}
});
});
}
16 changes: 10 additions & 6 deletions src/core/render/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,14 +326,18 @@ export function renderMixin(proto) {
{
compiler: this.compiler,
raw: result,
beforeEmbed: (tokens, done) =>
callHook(this, 'beforeEmbed', tokens, done),
},
tokens => {
html = this.compiler.compile(tokens);
html = this.isRemoteUrl
? DOMPurify.sanitize(html, { ADD_TAGS: ['script'] })
: html;
callback();
next();
callHook(this, 'afterEmbed', tokens, updatedTokens => {
html = this.compiler.compile(updatedTokens);
html = this.isRemoteUrl
? DOMPurify.sanitize(html, { ADD_TAGS: ['script'] })
: html;
callback();
next();
});
}
);
}
Expand Down
2 changes: 2 additions & 0 deletions test/integration/docsify.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Sidenote, we need better hooks tests.

Copy link
Member

@trusktr trusktr Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Expand Down