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

add new "substitute" behavior #21

Closed
wants to merge 1 commit into from
Closed

Conversation

chrisweb
Copy link
Contributor

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This PR is related to the ticket #20 I opened earlier, it serves as proof of concept for the idea I mentioned in the ticket and it shows what an impact the change would have on the current code base

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Oct 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b968c99) 100.00% compared to head (8fb6455) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #21   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          261       289   +28     
=========================================
+ Hits           261       289   +28     
Files Coverage Δ
lib/index.js 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wooorm
Copy link
Member

wooorm commented Nov 6, 2023

Hey! Thank you for working on this. Appreciate all the thinking you did.

There’s one gotcha with their new approach: links in headings.

## a [b](c) d
<h2 tabindex="-1" id="user-content-a-b-d" dir="auto">
  <a class="heading-link" href="#a-b-d">a </a>
  <a href="/wooorm/private/blob/main/c">b</a>
  d
  <svg ...></svg>
</h2>
## [a](b) c
<h2 tabindex="-1" id="user-content-a-c" dir="auto">
  <a class="heading-link" href="#a-c"></a>
  <a href="/wooorm/private/blob/main/b">a</a>
  c
  <svg ...></svg>
</h2>

Honestly I think it’s pretty wild that they completely forgot about links...
Now, I’m pretty sure at they take a naïve approach and wrap all content in a link, and serialize it. But then when a browser sees the second <a>, it assumes an </a> was there before it.

@wooorm
Copy link
Member

wooorm commented Nov 6, 2023

OK, so I think two things are missing:

  • support for adding content inside the link, which is basically content, but that is currently as you noted not supported with 'wrap'.
  • support for properties on the heading

@wooorm
Copy link
Member

wooorm commented Nov 6, 2023

What do you think of this to allow content with wrap?

@@ -96,7 +96,7 @@ export default function rehypeAutolinkHeadings(options) {
   const settings = options || emptyOptions
   let props = settings.properties
   const behavior = settings.behavior || 'prepend'
-  const content = settings.content || contentDefaults
+  const content = settings.content
   const group = settings.group
   const is = convertElement(settings.test)
 
@@ -133,7 +133,7 @@ export default function rehypeAutolinkHeadings(options) {
 
   /** @type {import('unist-util-visit').Visitor<Element>} */
   function inject(node) {
-    const children = toChildren(content, node)
+    const children = toChildren(content || contentDefaults, node)
     node.children[behavior === 'prepend' ? 'unshift' : 'push'](
       create(node, toProperties(props, node), children)
     )
@@ -146,7 +146,7 @@ export default function rehypeAutolinkHeadings(options) {
     /* c8 ignore next -- uncommon */
     if (typeof index !== 'number' || !parent) return
 
-    const children = toChildren(content, node)
+    const children = toChildren(content || contentDefaults, node)
     const link = create(node, toProperties(props, node), children)
     let nodes = behavior === 'before' ? [link, node] : [node, link]
 
@@ -166,7 +166,19 @@ export default function rehypeAutolinkHeadings(options) {
 
   /** @type {import('unist-util-visit').Visitor<Element>} */
   function wrap(node) {
-    node.children = [create(node, toProperties(props, node), node.children)]
+    let children = node.children
+
+    if (typeof content === 'function') {
+      const copy = content(node)
+      children = Array.isArray(copy) ? copy : [copy]
+    } else if (content) {
+      const copy = clone(content)
+      children = Array.isArray(copy)
+        ? [...node.children, ...copy]
+        : [...node.children, copy]
+    }
+
+    node.children = [create(node, toProperties(props, node), children)]
     return [SKIP]
   }
 }

@chrisweb
Copy link
Contributor Author

chrisweb commented Nov 7, 2023

Hello 👋

You welcome, thank you for reviewing this and all the stuff you do here, I spent quite some time recently with your packages and I love what can be done using them

Oh wow, yes you are right, anchors in headings are problematic on github and they also don't work in the code I suggested

What do you think of this to allow content with wrap?

Yes they way you did it works great, I did a bunch of tests including with anchors

The only thing is that if you have an anchor in the heading, then it will produce an anchor in anchor (which is invalid):

/**
 * @typedef {import('rehype-autolink-headings').Options} Options
 */

import {rehype} from 'rehype'
import rehypeAutolinkHeadings from 'rehype-autolink-headings'

const file = await rehype()
  .data('settings', {fragment: true})
  .use(
    rehypeAutolinkHeadings,
    /** @type {Options} */ {
      behavior: 'wrap',
    }
  )
  .process('<h1 id="foo">a <a href="c">b</a> d</h1>')

console.log(String(file))

will output:

<h1 id="foo"><a href="#foo">a <a href="c">b</a> d</a></h1>

but because content can be a function, the dev can solve this the way he wants by either doing a toString on the node or by use a more complex function that checks if there is an anchor in the children and if there is do something about it

how do you prefer we proceed, do we close this PR and you commit your code or should I revert my changes and add your code, or I can do a completely new PR if this saves you time?

wooorm added a commit that referenced this pull request Nov 8, 2023
Related-to: GH-20.
Related-to: GH-21.

Co-authored-by: Chris Weber <chris963@gmail.com>
@wooorm wooorm closed this in 01133a3 Nov 8, 2023

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Nov 8, 2023
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Nov 8, 2023
@wooorm
Copy link
Member

wooorm commented Nov 8, 2023

Thanks!
Added the new needed option, and allowed content with wrap.
Links in headings might indeed be nice to solve. It’s less of a problem for me when GH also has that bug!

@chrisweb
Copy link
Contributor Author

chrisweb commented Nov 8, 2023

Thank you @wooorm for this new v7.1.0 release with improved wrap functionality, oh wow you also updated the docs and tests, nice 😃

I just tested it in my project, works like a charm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants