refactor: rewrite getAndRemoveConfig(str) function #2472

Koooooo-7 · 2024-07-22T14:47:22Z

Summary

Reactor getAndRemoveConfig(str) function to a generic Lexer instead of a complex regex.
Correct current behavior and indicate docsify needs to resolve valid config keys. Warning invalid configs also.

Remove to use a tricky returned title as config options.
Instead, new KEY_appened_props introduced.

It will let the config parse as :KEY=VALUE append_props..., each config processor can handle its own logic
for the Value and append_props. e.g.

:class=classA classB classC :include
The classA assigns to class=classA, the rest of classB classC add to class_appened_props.

config

{
  class: "classA",
  class_appened_props: "classB classC",
  include: true
}

Which can support multi values configs of one key such as #2471 .

Related issue, if any:

What kind of change does this PR introduce?

Refactor

For any code change,

Related documentation has been updated, if needed
Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

Yes
No

Tested in the following browsers:

Chrome
Firefox
Safari
Edge

Reactor getAndRemoveConfig function to a generic lexer instead of a complex regex. Correctly the behavior and only resolve valid configs. Warning invalid configs also.

vercel · 2024-07-22T14:47:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docsify-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 28, 2024 8:56am

jhildenbiddle · 2024-07-22T17:25:45Z

@Koooooo-7 --

How does this handle multiple attributes when multiple values are provided?

[Text](page.md ':target=_blank :class=foo bar')
[Text](page.md ':class=foo bar :target=_blank ')

I would have assumed multiple values would need to be wrapped in quotes:

[Text](page.md ':target=_blank :class="foo bar"')
[Text](page.md ':class="foo bar" :target=_blank ')

Can we fix the "strict quote at star/end the configs" by trimming spaces and line endings from the start and end of the string found between quotes? If we don't, a single extra space could throw errors which may be hard to track down.
Why not just apply all attribute names and values regardless of whether they are "known"? This would allow people to add things like data attributes to elements in markdown.
```
[Text](page.md ':foo=bar :data-baz=buzz')
```
```
<a href="..." foo="bar" data-baz="buzz">Text</a>
```

paulhibbitts · 2024-07-22T17:51:01Z

Thanks very much @Koooooo-7 for this! I've put my original CodeSandbox example online with your Docsify Preview build and @jhildenbiddle examples to help with testing - looking good so far 🙂 https://paulhibbitts.github.io/docsify-v5-preview/#/test-3

Koooooo-7 · 2024-07-23T04:42:11Z

@Koooooo-7 --
How does this handle multiple attributes when multiple values are provided?
[Text](page.md ':target=_blank :class=foo bar')
[Text](page.md ':class=foo bar :target=_blank ')

It will be resolve to

{
  target: "_blank",
  class: foo,
  class_appened_props: "bar"
}

Cos the append props will look back to the first leading KEY.

Can we fix the "strict quote at star/end the configs" by trimming spaces and line endings from the start and end of the string found between quotes? If we don't, a single extra space could throw errors which may be hard to track down.

I see, will refactor to retrieve the closest pre/next '/" instead.

Why not just apply all attribute names and values regardless of whether they are "known"? This would allow people to add things like data attributes to elements in markdown.
[Text](page.md ':foo=bar :data-baz=buzz')

Gotcha. Will update to clean the whole config blocks and only retrieve docsify valid config tokens.

Koooooo-7 · 2024-07-23T14:31:05Z

Hi @jhildenbiddle ---
Already update:

Support loose quote ' :class=any :include '
Support unknown attributes ':class=foo :foo=bar'

And I add more test cases and comments on those cases we discussed.

paulhibbitts · 2024-07-23T15:12:32Z

Thanks so much @Koooooo-7 , this is going to be a really nice improvement 🙂 I've updated my online Docsify Preview build with your latest Preview and things look good! https://paulhibbitts.github.io/docsify-v5-preview/#/test-3

Koooooo-7 · 2024-07-23T15:13:48Z

Thanks so much @Koooooo-7 , this is going to be a really nice improvement 🙂 I've updated my online Docsify Preview build with your latest Preview and things look good! https://paulhibbitts.github.io/docsify-v5-preview/#/test-3

Thx for you live preview for the changes !!!

jhildenbiddle

Suggestion on this:

How does this handle multiple attributes when multiple values are provided?
[Text](page.md ':target=_blank :class=foo bar')
[Text](page.md ':class=foo bar :target=_blank ')
It will be resolve to
{
  target: "_blank",
  class: foo,
  class_appened_props: "bar"
}

Instead of capturing multiple values using class and class_appended_props properties, why not require multiple values to be wrapped in quotes? This seems more natural since it is how attributes and values work in HTML. It will also make it easier to read multiple attribute values because all values are contained in a single property.

For example:

[Text](page.md ':foo :target=_blank :class="bar baz" This is the title')

{
  foo: true,
  target: "_blank",
  class: "bar baz",
  title: "This is the title" // links support title attribute in markdown
}

The rules would then be:

All attributes must start with :
Attributes without values are valid (:foo)
Quotes are optional for single values (:target=_blank or :target="_blank")
Quotes are required for multiple attribute values (:class="bar baz"')

The question is then if/how we want to handle stray, unquoted values. Consider the following example:

[Text](page.md ':foo=val1 val2 :bar=val3 val4'). // Missing quotes around multiple values

I would expect this to produce the following:

{
  foo: "val1",
  bar: "val3",
  title: "val4",
}

What happens to val2?

The easiest thing to do would be to just drop val2 because according to the rules this is neither an attribute (does not start with :) or an attribute value.

Thoughts?

Koooooo-7 · 2024-07-25T17:28:00Z

Instead of capturing multiple values using class and class_appended_props properties, why not require multiple values to be wrapped in quotes? This seems more natural since it is how attributes and values work in HTML. It will also make it easier to read multiple attribute values because all values are contained in a single property.

For example:
[Text](page.md ':foo :target=_blank :class="bar baz" This is the title')
{
  foo: true,
  target: "_blank",
  class: "bar baz",
  title: "This is the title" // links support title attribute in markdown
}
The rules would then be:

All attributes must start with :

Attributes without values are valid (:foo)

Quotes are optional for single values (:target=_blank or :target="_blank")

Quotes are required for multiple attribute values (:class="bar baz"')

That would be more clearly. but it brings a breaking change for all the config rules for now. So I choose a compromise way to make it and doesn't breaking anything to get ride of the regex.

Besides, the config of docsify is not that consistent.
On the config class, it looks they should be as the same value.
But such as :type=code js or :ignore title.
It's not a same thing to the config Key, more likely a random appendix for current config depends on what the config support.

The question is then if/how we want to handle stray, unquoted values. Consider the following example:
[Text](page.md ':foo=val1 val2 :bar=val3 val4'). // Missing quotes around multiple values
I would expect this to produce the following:
{
  foo: "val1",
  bar: "val3",
  title: "val4",
}
What happens to val2?

The easiest thing to do would be to just drop val2 because according to the rules this is neither an attribute (does not start with :) or an attribute value.

Thoughts?

It is hard to say a data attribute nor a docsify config within a string.

If we decided include a breaking change on this. I think we can introduce a docsify config block. e.g.

[Text](page.md ' docsify=[:foo :target=_blank :class=bar baz]  This is the title')

Then, we have a clear line to distinguish between the docsify part and custom made for markdown owned.

jhildenbiddle · 2024-07-27T14:57:37Z

That would be more clearly. but it brings a breaking change for all the config rules for now. So I choose a compromise way to make it and doesn't breaking anything to get ride of the regex.

Besides, the config of docsify is not that consistent.

Makes sense. I didn't realize that the attribute parser is inconsistent across tags. I appreciate trying to not introduce a non-breaking change, but perhaps this is the opportunity make attribute parsing consistent in docsify for all markdown tags that support it. See below...

If we decided include a breaking change on this. I think we can introduce a docsify config block. e.g.
[Text](page.md ' docsify=[:foo :target=_blank :class=bar baz]  This is the title')
Then, we have a clear line to distinguish between the docsify part and custom made for markdown owned.

Can we add something like the proposed "block" of attributes above as a new v5 feature but retain the existing :prop=val behavior for backwards compatibility? This would allow us to update our docs with the new "preferred" method alongside the older :prop=name method, but note in the docs that the older method is deprecated in v5 and will be removed in v6.

For example, we can drop the docsify= and use the : as the start of the pattern:

[Text](page.md ':[foo target="_blank" class="bar baz"]  This is the title')

Alternatively, we could use single or double brackets to start/end the pattern which would make it easy for us to parse using JSON.stringify once the string was extracted:

[Text](page.md '{foo: true, target: "_blank" class: "bar baz"}  This is the title')

[Text](page.md '{{foo: true, target: "_blank" class: "bar baz"}}  This is the title')

If we go this route, I would propose we not allow mixing new and old attribute styles like this:

[Text](page.md '{{foo: true, class: "bar baz"}} :target=_blank This is the title')

Doing this promotes continued use of the old :prop=name style. When this happens, we can either handle both old and new styles with a warning in the console, or we can process only the new style and ignore the older :prop style.

This seems like a better approach than updating the getAndRemoveConfig function with new capabilities that only work in some scenarios and don't address Docsify's inconsistencies in markdown attribute parsing.

Koooooo-7 · 2024-07-28T08:12:15Z

--- @jhildenbiddle

This seems like a better approach than updating the getAndRemoveConfig function with new capabilities that only work in some scenarios and don't address Docsify's inconsistencies in markdown attribute parsing.

Yes, there is a plain refactor of current implementation without any changes.
The purpose of the changes for me is to refactor the regex to a easy to handle lexer first.
Then we could update it to a new configs rules and strict to what we want as a docsify configuration.

Based on this refactor doesn't breaking any current behavior, let's think about the new config functions.

Can we add something like the proposed "block" of attributes above as a new v5 feature but retain the existing :prop=val behavior for backwards compatibility? This would allow us to update our docs with the new "preferred" method alongside the older :prop=name method, but note in the docs that the older method is deprecated in v5 and will be removed in v6.

First of all, I agree the notes way to reminder users to update its configs if needs in v5 and future, give user a buffer to know the changes. ✅

About how to do the redesign about the docsify helper configs, tbh, I don't have a very clear assumption on this for now.
Don't worry about what it should be in future and what it is for now. I believe we can handle them in the v5 with compatibility.

On your ideas.

A:

[Text](page.md ':[foo target="_blank" class="bar baz"]  This is the title')

It is okay, :[ is the config heading and the :[] has a clear config block.
The extra effort for us is the quote since it means user can config in:

single quote  ------   ':[foo target="_blank" class="bar baz"]  This is the title'
double quote ------  ":[foo target="_blank" class='bar baz']  This is the title"

There contains a extra quote validation between the markdown config block and the "values group". Although we have to face it currently, I suppose we'd better drop it if we could.
How about this:

[Text](page.md ':[ :foo :target=_blank :class=bar baz] This is the title')

the :[] is the config block, :[ is the config heading.
keep the :Key rules for each config key.
remove the ", all the values after :Key= will append to the leading :Key until to the end or meet another :Key2.

B:

Alternatively, we could use single or double brackets to start/end the pattern which would make it easy for us to parse using JSON.stringify once the string was extracted:

[Text](page.md '{foo: true, target: "_blank" class: "bar baz"}  This is the title')
[Text](page.md '{{foo: true, target: "_blank" class: "bar baz"}}  This is the title')

Personally, I think this is a good idea. what we need we is getting the {} block and do a JSON deserialization directly. 💯
The only concern is on the modern mvvm front-end libs. Almost all use the {{ msg }} as a value placeholder.
I'm not quite sure if we use the {}/{{}} could be a problem for user integration or something.
If not, I prefer this one. 👍

jhildenbiddle · 2024-07-29T18:00:42Z

I think both styles have pros and cons:

Colon style (:foo=bar) is clearly unique to Docsify and therefore should not impact utilities which interpret the brackets to as JavaScript like MDX. Colon style also allows applying valueless attributes:
```
[Text](page.md ':[data-foo] My Title)` = `<a href="..." data-foo title="My Title">
```
Bracket style ({..}or {{...}}) is easier for us to parse and familiar to developers.

Here is some additional feedback on each style:

Colon Style

[Text](page.md ':[foo target="_blank" class="bar baz"]  This is the title')
It is okay, :[ is the config heading and the :[] has a clear config block. The extra effort for us is the quote since it means user can config in:
single quote  ------   ':[foo target="_blank" class="bar baz"]  This is the title'
double quote ------  ":[foo target="_blank" class='bar baz']  This is the title"

As you mention, this is something we already have to handle today. All of the following examples are valid regardless of how we handle attributes:

[Text](page.md 'This is the title')
[Text](page.md "This is the title")
[Text](page.md "This is the title with 'single quotes' and a \"double quotes\"")

Even if we went with "bracket style", we'd still have to handle single vs. double quotes:

[Text](page.md '{foo: "bar"} This is the title')
[Text](page.md "{foo: 'bar'} This is the title")
[Text](page.md "{foo: 'bar', baz: \"buzz\"} This is the title with 'single quotes' and a \"double quotes\"")

There contains a extra quote validation between the markdown config block and the "values group". Although we have to face it currently, I suppose we'd better drop it if we could. How about this:

[Text](page.md ':[ :foo :target=_blank :class=bar baz] This is the title')

This format feels like we're forcing users to work around how our parser works instead of designing our parser to work the way that makes the most sense to users.

The closer we can get to the familiar name="value" HTML format or the name: "value" JS/JSON format, the better. The point of the "config block" format below is to align with this goal:

`[Text](page.md ':[foo target="_blank" class="bar baz"] This is the title')`

Bracket Style

Alternatively, we could use single or double brackets to start/end the pattern which would make it easy for us to parse using JSON.stringify once the string was extracted:
[Text](page.md '{foo: true, target: "_blank" class: "bar baz"}  This is the title')
[Text](page.md '{{foo: true, target: "_blank" class: "bar baz"}}  This is the title')
Personally, I think this is a good idea. what we need we is getting the {} block and do a JSON deserialization directly. 💯 The only concern is on the modern mvvm front-end libs. Almost all use the {{ msg }} as a value placeholder. I'm not quite sure if we use the {}/{{}} could be a problem for user integration or something. If not, I prefer this one. 👍

My understanding is that single brackets ({...}) are typically used for string interpolation while double brackets ({{...}}) typically indicate JavaScript to be parsed/executed. Given that, I think double-brackets are the better option:

[Text](page.md '{{foo: true, target: "_blank" class: "bar baz"}}  This is the title')

Final Thoughts

I am open to adding either colon style or bracket style as a new v5 feature, however I do not think we should promote the ability to specify multiple value with the previous :name=value format. If we are going to introduce a new method of assigning attributes and values because we believe it is better than the old way, then let's encourage users to adopt the new method so we can deprecate (and eventually remove) the older method.

If we go this route, do the changes in this PR (regex => lexer) still make sense? If so, we can keep them. If not, then let's focus on getting the new-and-improved format in place instead of "improving" code which we hope to remove in the next release.

paulhibbitts · 2024-07-29T18:56:23Z

Thanks so much @Koooooo-7 @jhildenbiddle for working this out some more. My feeling are also mixed re: colon or brackets style... the use of [] is already in Markdown but that also means they are now used in multiple ways in a single line of Markdown which might be less than clear and not expected.

To confirm, if this change went ahead existing single class assignments would still work - correct?

jhildenbiddle · 2024-07-29T19:15:52Z

To confirm, if this change went ahead existing single class assignments would still work - correct?

Correct. My proposal is that keep the current behavior and add new functionality via a new attribute/value format.

paulhibbitts · 2024-07-29T21:22:26Z

BTW, after coming across other examples of Markdown dialect classes (https://commonmark.thephpleague.com/2.4/extensions/attributes/ and https://masteringlaravel.io/daily/2023-10-05-applying-a-class-to-a-markdown-link) I am now tending even more to the use of {}. These examples were new to me 🙂

Koooooo-7 · 2024-07-30T03:40:02Z

--- @jhildenbiddle

Final Thoughts

I am open to adding either colon style or bracket style as a new v5 feature, however I do not think we should promote the ability to specify multiple value with the previous :name=value format. If we are going to introduce a new method of assigning attributes and values because we believe it is better than the old way, then let's encourage users to adopt the new method so we can deprecate (and eventually remove) the older method.

I think the {{}} if acceptable for me when the "/' inside the markdown contents is fine, thx for the details.

If we go this route, do the changes in this PR (regex => lexer) still make sense? If so, we can keep them. If not, then let's focus on getting the new-and-improved format in place instead of "improving" code which we hope to remove in the next release.

If we decide to replace it as a brand-new configs styles for users, I think this improving is not that necessary since we will replace it totally instead of a updating.
Instead, I will raise an issue on this for the new config ways in docsify.

Koooooo-7 · 2024-07-30T04:43:36Z

close via #2476 2476

jhildenbiddle · 2024-07-30T16:50:09Z

@paulhibbitts --

The main issue with the CommonMark extension format used in the links you provided is that it makes the markdown far less portable. If the markdown parser is does not support the extended syntax, the {..} will appear in the rendered output.

For example, the following markdown:

[Some Text](some-url){ .foo .bar }

Will render as following on GitHub:

With either of the formats being proposed above, the extended syntax will not render as text but will be visible on hover as part of the title attribute:

Document portability is arguably the most critical feature of markdown, so I personally am not a fan of extensions which result in unsupported syntax rending as content when moved between markdown environments.

Just FYI. :)

paulhibbitts · 2024-07-30T16:59:21Z

Oh thanks for the additional info @jhildenbiddle , I was not aware of that factor in regards to the use of single or double brackets!

ps - as a follow up test, it looks like [Text](page.md ':[foo target="_blank" class="bar baz"] This is the title') renders in GitHub with the additional formatting displayed as a hover https://github.com/paulhibbitts/docsify-v5-preview/blob/main/docs/test-3.md

Koooooo-7 · 2024-07-31T15:55:49Z

ps - as a follow up test, it looks like [Text](page.md ':[foo target="_blank" class="bar baz"] This is the title') renders in GitHub with the additional formatting displayed as a hover https://github.com/paulhibbitts/docsify-v5-preview/blob/main/docs/test-3.md

Yes, cos we haven't support it yet.

jhildenbiddle · 2024-07-31T16:59:07Z

@paulhibbitts ”
ps - as a follow up test, it looks like [Text](page.md ':[foo target="_blank" class="bar baz"] This is the title') renders in GitHub with the additional formatting displayed as a hover https://github.com/paulhibbitts/docsify-v5-preview/blob/main/docs/test-3.md

Correct. The markdown standard states that anything contained within the quotes will be included as part of the title attribute. It won’t matter which format we choose. As long as out “configuration block” is contained within the quotes, it will be included as part of the title value when the markdown is viewed in an environment other than docsify (like GitHub).

update: rewrite getAndRemove config

674e88b

Reactor getAndRemoveConfig function to a generic lexer instead of a complex regex. Correctly the behavior and only resolve valid configs. Warning invalid configs also.

vercel bot deployed to Preview July 22, 2024 14:47 View deployment

Merge branch 'develop' into update-rewrite-gr-config

ab30e2c

vercel bot deployed to Preview July 22, 2024 14:48 View deployment

fix: lint

cd38063

vercel bot deployed to Preview July 22, 2024 15:04 View deployment

fix: lint

1aa88b2

vercel bot deployed to Preview July 22, 2024 15:10 View deployment

Koooooo-7 marked this pull request as ready for review July 22, 2024 15:13

Koooooo-7 requested review from jhildenbiddle, trusktr, sy-records and paulhibbitts July 22, 2024 15:14

chore: typo

73dc9f7

vercel bot deployed to Preview July 22, 2024 16:28 View deployment

sy-records changed the title ~~update: rewrite getAndRemove config~~ refactor: rewrite getAndRemove config Jul 23, 2024

update: review

b0218df

Koooooo-7 changed the title ~~refactor: rewrite getAndRemove config~~ refactor: rewrite getAndRemoveConfig(str) function Jul 23, 2024

vercel bot deployed to Preview July 23, 2024 14:25 View deployment

chore:typo

89bc77e

vercel bot deployed to Preview July 23, 2024 14:53 View deployment

chore: polish

56cdeab

vercel bot deployed to Preview July 24, 2024 12:33 View deployment

jhildenbiddle requested changes Jul 25, 2024

View reviewed changes

Merge branch 'develop' into update-rewrite-gr-config

5ea62fd

vercel bot deployed to Preview July 28, 2024 08:56 View deployment

Koooooo-7 mentioned this pull request Jul 30, 2024

Introduce a new docsify config for markdown enrichment #2476

Open

Koooooo-7 closed this Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rewrite getAndRemoveConfig(str) function #2472

refactor: rewrite getAndRemoveConfig(str) function #2472

Koooooo-7 commented Jul 22, 2024 •

edited

Loading

vercel bot commented Jul 22, 2024 •

edited

Loading

jhildenbiddle commented Jul 22, 2024 •

edited

Loading

paulhibbitts commented Jul 22, 2024

Koooooo-7 commented Jul 23, 2024 •

edited

Loading

Koooooo-7 commented Jul 23, 2024

paulhibbitts commented Jul 23, 2024

Koooooo-7 commented Jul 23, 2024

jhildenbiddle left a comment

Koooooo-7 commented Jul 25, 2024 •

edited

Loading

jhildenbiddle commented Jul 27, 2024

Koooooo-7 commented Jul 28, 2024

jhildenbiddle commented Jul 29, 2024

paulhibbitts commented Jul 29, 2024

jhildenbiddle commented Jul 29, 2024

paulhibbitts commented Jul 29, 2024

Koooooo-7 commented Jul 30, 2024

Final Thoughts

Koooooo-7 commented Jul 30, 2024

jhildenbiddle commented Jul 30, 2024

paulhibbitts commented Jul 30, 2024 •

edited

Loading

Koooooo-7 commented Jul 31, 2024

jhildenbiddle commented Jul 31, 2024

refactor: rewrite getAndRemoveConfig(str) function #2472

refactor: rewrite getAndRemoveConfig(str) function #2472

Conversation

Koooooo-7 commented Jul 22, 2024 • edited Loading

Summary

Related issue, if any:

What kind of change does this PR introduce?

For any code change,

Does this PR introduce a breaking change?

Tested in the following browsers:

vercel bot commented Jul 22, 2024 • edited Loading

jhildenbiddle commented Jul 22, 2024 • edited Loading

paulhibbitts commented Jul 22, 2024

Koooooo-7 commented Jul 23, 2024 • edited Loading

Koooooo-7 commented Jul 23, 2024

paulhibbitts commented Jul 23, 2024

Koooooo-7 commented Jul 23, 2024

jhildenbiddle left a comment

Choose a reason for hiding this comment

Koooooo-7 commented Jul 25, 2024 • edited Loading

jhildenbiddle commented Jul 27, 2024

Koooooo-7 commented Jul 28, 2024

jhildenbiddle commented Jul 29, 2024

Colon Style

Bracket Style

Final Thoughts

paulhibbitts commented Jul 29, 2024

jhildenbiddle commented Jul 29, 2024

paulhibbitts commented Jul 29, 2024

Koooooo-7 commented Jul 30, 2024

Final Thoughts

Koooooo-7 commented Jul 30, 2024

jhildenbiddle commented Jul 30, 2024

paulhibbitts commented Jul 30, 2024 • edited Loading

Koooooo-7 commented Jul 31, 2024

jhildenbiddle commented Jul 31, 2024

Koooooo-7 commented Jul 22, 2024 •

edited

Loading

vercel bot commented Jul 22, 2024 •

edited

Loading

jhildenbiddle commented Jul 22, 2024 •

edited

Loading

Koooooo-7 commented Jul 23, 2024 •

edited

Loading

Koooooo-7 commented Jul 25, 2024 •

edited

Loading

paulhibbitts commented Jul 30, 2024 •

edited

Loading