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

Serialize back a textDirective to its original form? #12

Closed
4 tasks done
slorber opened this issue Sep 22, 2023 · 4 comments
Closed
4 tasks done

Serialize back a textDirective to its original form? #12

slorber opened this issue Sep 22, 2023 · 4 comments
Labels
🙋 no/question This does not need any changes 👎 phase/no Post cannot or will not be acted on

Comments

@slorber
Copy link

slorber commented Sep 22, 2023

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?

@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 Sep 22, 2023
@wooorm
Copy link
Member

wooorm commented Sep 22, 2023

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

@ChristianMurphy ChristianMurphy added the 🙋 no/question This does not need any changes label Sep 25, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Sep 25, 2023
@slorber
Copy link
Author

slorber commented Oct 9, 2023

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:

  • is this even a good idea to implement this? (vs warning, as you suggested)
  • if it is a good idea, shouldn't there be an official package to handle this more officially, instead of me implementing alone on Docusaurus?

@slorber
Copy link
Author

slorber commented Oct 24, 2023

So we implemented the warning plugin here: facebook/docusaurus#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 no/question This does not need any changes 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

3 participants