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.

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

feat(core/extensions): add extensions API #3320

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Feb 19, 2021

Closes #1976

Support 4 extension hooks: before-all (preProcess), after-all (postProcess), before-markdown and after-markdown.
Support two extension utils: showError and showWarning.

TODO:

  • investigate more hooks to start with
  • investigate more utils to start with
  • write tests

Support two extension hooks: before-markdown and after-markdown.
Support two extension utils: showError and showWarning.
@marcoscaceres
Copy link
Member

I think this is still risky... I'd prefer people just user preProcess and postProcess. Hooking into anything makes things fragile, as we've seen.

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Feb 23, 2021

Hooking into anything makes things fragile, as we've seen.

I understand the concern, but I think the previous hooks were fragile as we exposed them everywhere without much thought. If we limit what we expose, we can have a much more powerful and stable API. Also, when we think we need to remove some hook, we can deprecate them (as a breaking change), as we know what's exposed where.

As an example, if you look at respec.org/docs/src.html#L29-L36 (and respec.org/docs/src.html#L98-L128 in particular), you'll see what hacks I had to make to overcome the limitations of preProcess and postProcess 😅

@marcoscaceres
Copy link
Member

As an example, if you look at respec.org/docs/src.html#L29-L36 (and respec.org/docs/src.html#L98-L128 in particular), you'll see what hacks I had to make to overcome the limitations of preProcess and postProcess 😅

I see... what's a rough sketch of what that would look like with the new api?

@saschanaz
Copy link
Member

Looks cool. Which specs would use the markdown hooks?

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Mar 21, 2021

Haven't tested real specs that would like to use after-markdown hook, but respec.org/docs/src.html#L115-L125 could use it like this:

diff: docs/src.html
diff --git a/static/docs/src.html b/static/docs/src.html
index 9c5375d..03418a3 100644
--- a/static/docs/src.html
+++ b/static/docs/src.html
@@ -34,6 +34,26 @@
           postProcessEnhance,
           cleanup,
         ],
+        extensions: [
+          {
+            name: "note-advisement",
+            on: "after-markdown",
+            run() {
+              for (const p of document.querySelectorAll("p")) {
+                if (p.firstElementChild?.localName === "strong") {
+                  const text = p.firstElementChild.textContent.trimStart();
+                  if (text.startsWith("Note:")) {
+                    p.classList.add("note");
+                    p.firstElementChild.remove();
+                  } else if (text.startsWith("Warning:")) {
+                    p.classList.add("advisement");
+                    p.firstElementChild.remove();
+                  }
+                }
+              }
+            },
+          },
+        ],
         logos: [
           {
             src: "/respec-logo.png",
@@ -112,18 +132,6 @@
           .replace(/\`\|(\w+)/g, `\`\\|${ZERO_WIDTH_SPACE}$1`)
           .replace(/(\w+)\|\`/g, `$1${ZERO_WIDTH_SPACE}\\|\``);
 
-        // Add .note and .advisement classes based on line prefix
-        content = content
-          .split("\n")
-          .map(line => {
-            if (/^.{0,5}\s*Warning:/.test(line))
-              return `<div class="advisement">\n\n${line}</div>`;
-            if (/^.{0,5}\s*Note:/.test(line))
-              return `<div class="note">\n\n${line}</div>`;
-            return line;
-          })
-          .join("\n");
-
         return content;
       }

With a potential after-inline-processing hook (which runs before linker), docs/src.html#L99-L113 will simplify a lot.

Similarly, if we could run fixLinks(), say, on: "after-linker", we would get access to local-refs-exist linker in a better way. Currently linters run after doc.respec.ready, which means we can miss linter warnings in respec2html (to workaround this, we wait for 1 second after respec.ready which is problematic in itself).

Will test with some other real specs with potential hooks and update here.

(PS: Missed notifications as I accidentally unsubscribed from this PR).

Edits:

  • Many ARIA/A11Y, JSON-LD and other specs could benefit from after-linker event. They currently use pubsubhub.end["core/link-to-dfn"]. Maybe we can expose after-local-linker and after-external-linker (need to be careful of course).
  • A lot of specs could use utils.showError() and utils.showWarning(). Though, they could as well be added to preProcess,postProcess, and isn't an endorsement for new extension API.

@marcoscaceres
Copy link
Member

marcoscaceres commented Mar 23, 2021

I still think this is going to be extremely footgunny.

Currently linters run after doc.respec.ready, which means we can miss linter warnings in respec2html (to workaround this, we wait for 1 second after respec.ready which is problematic in itself).

Yeah, we should change the linters back into normal plugins as you suggested elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New API for external plugins
3 participants