remarkjs / remark-directive

remark plugin to support directives
https://remark.js.org
MIT License
250 stars 15 forks source link

Serialize back a textDirective to its original form? #12

Closed slorber closed 12 months ago

slorber commented 1 year ago

Initial checklist

Problem

Users sometimes need to use :textDirective in their content, without the intent of creating a textDirective.

Some examples found in the React-Native website:

Check out the video from [AWS re:Invent](https://www.youtube.com/watch?v=vAjf3lyjf8c).

Creates an object that represent android theme's default background for borderless selectable elements (?android:attr/selectableItemBackgroundBorderless). Available on android API level 21+. `rippleRadius` parameter controls the radius of the ripple effect.

- `alert` :boolean
- `badge` :boolean
- `sound` :boolean

:Invent, :attr and :boolean being now parsed as textDirective, this leads to these content requiring to be refactored so that they keep being displayed as before turning directives on.

Otherwise this is what happens:

image

image

image


Of course we can escape \: or use the HTML code :, but we can only do so if we notice the visual regression in the first place, that is quite easy to miss for large content sites ;) I only caught these little issues because I use an automated visual regression workflow.

Solution

I don't really have a solution, and not even sure it's the correct repo to discuss this, but:

This:

A :test 

Is rendered as:

A

If we could make it render as A :test instead, I think it would be much less likely to produce the unintended side effects described above.

Alternatives

This can probably be fixed in userland by writing a remark plugin that transforms unhandled textDirective nodes:

{
  "type": "textDirective",
  "name": "test",
  "attributes": {},
  "children": []
}

to

{
  "type": "text",
  "value": ":test"
}

I wanted to open this issue to discuss if it's a good idea, and if it makes sense to generalize this so that others can benefit from this behavior change?

wooorm commented 1 year ago

but we can only do so if we notice the visual regression in the first place, that is quite easy to miss for large content sites ;) I only caught these little issues because I use an automated visual regression workflow.

There is something else: this is a generic syntax extension, that could mean anything, on any site. markdown parsers know that these things are components, but they don’t need to know what those components mean.

For example, you could have a ::docu-youtube component, that means something to Docusaurus, but when displaying markdown on GH, it wouldn’t know what that means, just that it means a component, and it could show the data (e.g., attributes), sort of similar to how they display YAML frontmatter. I understand this as one of the goals of directives: platforms/syntax highlighters known it is a component.

Here we’re talking about docusaurus as the “end target” of the components in markdown. I think that it is docusaurus’ task to warn about components that it knows nothing about. Only docusaurus knows that it supports the theoretical :docu-alert and :docu-info. And only it knows that perhaps a user added support for :boolean or not.

Primarly, I think you should warn about components you don’t handle.


You’re raising this issue here, remark-directive. This plugin’s responsibility is parsing and serializing, by integrating with remark-parse and remark-stringify. And AFAIK, it parses :x and serializes :x. So I don’t think there’s anything to do here? There’s no rendering happening here.

With rendering, you might mean by MDX? Or you are using something else as the final result (rehype-stringify?). Those tools are set up to ignore anything they don’t understand. And that includes directives.


Finally, everything with directives happens with plugins: you can handle :x if you want. I believe you are looking for something that turns :x into a literal :x, as in, as if it was written as \:x.

You can do that. Here’s some pseudo code I wrote off the top of my head to show a way to go about it:

export default function somePlainTextDirectives(options) {
    const names = (options || {}).names || []
    return function (tree) {
        visit(tree, function (node, index parent) {
            if (parent && typeof index === 'number' && node.type === 'textDirective' && names.includes(node.name)) {
                const xxx = toMarkdown(node, {
                    extensions: [directiveToMarkdown()]
                })
                parent.children[index] = {type: 'text', value: xxx}
            }
        })
    }
}
github-actions[bot] commented 12 months ago

Hi! Thanks for reaching out! Because we treat issues as our backlog, we close issues that are questions since they don’t represent a task to be completed.

See our support docs for how and where to ask questions.

Thanks, — bb

slorber commented 11 months ago

Thanks @wooorm and sorry for the late reply.

Primarly, I think you should warn about components you don’t handle.

Yes that seems like a reasonable solution.

There’s no rendering happening here.

Agree on that, it's just that I didn't really know where else I could open this issue 😅


With rendering, you might mean by MDX? Or you are using something else as the final result (rehype-stringify?). Those tools are set up to ignore anything they don’t understand. And that includes directives.

Finally, everything with directives happens with plugins: you can handle :x if you want. I believe you are looking for something that turns :x into a literal :x, as in, as if it was written as \:x.

Yes that's the idea. I think most users will only be affected by this behavior for text directives because it's relatively common to use :, cf AWS re:Invent.

However I was mostly wondering:

slorber commented 11 months ago

So we implemented the warning plugin here: https://github.com/facebook/docusaurus/pull/9394

We only serialize back the "simple text directives" like Re:invent and skip emitting a warning, as it's the most common case that users are likely to encounter. We don't serialize back Re:Invent[label]{age=42} for example because it's much less likely to occur in the wild unless it's a real mistake