-
-
Notifications
You must be signed in to change notification settings - Fork 624
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
fix: allow ] in filename code block #2169
Conversation
β Deploy Preview for nuxt-content canceled.
|
@@ -20,7 +20,7 @@ export function parseThematicBlock (lang: string) { | |||
|
|||
const language = lang.replace(/[{|[](.+)/, '').match(/^[^ \t]+(?=[ \t]|$)/) | |||
const highlightTokens = lang.match(/{([^}]*)}/) | |||
const filenameTokens = lang.match(/\[([^\]]*)\]/) | |||
const filenameTokens = lang.match(/\[(.*)\]/) /** Allow ']' in filename. @see https://regex101.com/r/Hm0t5m/1 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we used [^\]]
to allow using lists in the meta part.
Here's an example what would not work anymore if we match the longest string.
```language [filename]{hightlights} meta=[...]
It would match this content filename]{hightlights} meta=[...
as the filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we could use something like \]
in the markdown and prevent matching those somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh I understand. I do it too quickly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, that this might work: /\[((\\]|[^\]])*)\]/
This part (\\]|[^\]])*
matches zero or one or a sequence of: \]
or characters that are not ]
Only if our string has no more ]
after a \]
it terminates the match at a \]
.
So this works:
```ts [[...slug\].vue] meta=[]
=> [...slug\].vue
as the first group match.
Then we only have to replace \]
which in reality is \\]
with ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but if there is no meta, this regex is not working.
https://regex101.com/r/bG0Net/1
EDIT: User must set a , I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be all the necessary changes, but we'll see...
This is currently not really tested.
I tried both \\]
and \]
and it seems that we need the first variant with two backslashes to get one real backslash from the Markdown.
So, this works in the playground:
```ts [[...slug\\].vue] ...
@@ -20,7 +20,7 @@ export function parseThematicBlock (lang: string) { | |||
|
|||
const language = lang.replace(/[{|[](.+)/, '').match(/^[^ \t]+(?=[ \t]|$)/) | |||
const highlightTokens = lang.match(/{([^}]*)}/) | |||
const filenameTokens = lang.match(/\[([^\]]*)\]/) | |||
const filenameTokens = lang.match(/\[(.*)\]/) /** Allow ']' in filename. @see https://regex101.com/r/Hm0t5m/1 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const filenameTokens = lang.match(/\[(.*)\]/) /** Allow ']' in filename. @see https://regex101.com/r/Hm0t5m/1 */ | |
const filenameTokens = lang.match(/\[((\\]|[^\]])*)\]/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And because of GitHub, I'm not allowed to suggest changes out of some range:
Change for L29:
- filename: Array.isArray(filenameTokens) && filenameTokens[1] ? filenameTokens[1] : undefined,
+ filename: Array.isArray(filenameTokens) && filenameTokens[1] ? filenameTokens[1].replace(/\\]/g, ']') : undefined,
To remove the now redundant backslashes.
@@ -20,7 +20,7 @@ export function parseThematicBlock (lang: string) { | |||
|
|||
const language = lang.replace(/[{|[](.+)/, '').match(/^[^ \t]+(?=[ \t]|$)/) | |||
const highlightTokens = lang.match(/{([^}]*)}/) | |||
const filenameTokens = lang.match(/\[([^\]]*)\]/) | |||
const filenameTokens = lang.match(/\[(.*)\]/) /** Allow ']' in filename. @see https://regex101.com/r/Hm0t5m/1 */ | |||
const meta = lang.replace(/^\w*\s*(\[[^\]]*\]|\{[^}]*\})?\s*(\[[^\]]*\]|\{[^}]*\})?\s*/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const meta = lang.replace(/^\w*\s*(\[[^\]]*\]|\{[^}]*\})?\s*(\[[^\]]*\]|\{[^}]*\})?\s*/, '') | |
const meta = lang.replace(/^\w*\s*(\[((\\]|[^\]])*)\]|\{[^}]*\})?\s*(\[((\\]|[^\]])*)\]|\{[^}]*\})?\s*/, '') |
To replace the correct part, to have the meta left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactor this to be more understandable and to reuse previous regex.
I will also update documentation to add to warning about usage. |
β Live Preview ready!
|
β Live Preview ready!
|
const meta = lang.replace(/^\w*\s*(\[[^\]]*\]|\{[^}]*\})?\s*(\[[^\]]*\]|\{[^}]*\})?\s*/, '') | ||
const languageMatches = lang.replace(/[{|[](.+)/, '').match(/^[^ \t]+(?=[ \t]|$)/) | ||
const highlightTokensMatches = lang.match(/{([^}]*)}/) | ||
const filenameMatches = lang.match(/\[((\S)*)\]/) /** Allow ']' in filename. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that you don't want an escape character to have ]
in your filename, but we didn't want to depend on spaces. If we now change this, it could be a breaking change.
My variant from before would introduce an escape character (e.g. \\
) for ]
but
- would not need spaces after the last
]
that belongs to the filename - allow spaces in the filename.
You could not do this anymore:
```ts [filename]meta=[]
or
```ts [filename/title with spaces]
Spaces in the filename for example are already used in the nuxt/nuxt
docs. Some examples:
- https://github.com/nuxt/nuxt/blob/350788419c414317354de9900cd049fa750456cc/docs/7.migration/4.meta.md?plain=1#L27
- https://github.com/nuxt/nuxt/blob/350788419c414317354de9900cd049fa750456cc/docs/1.getting-started/5.routing.md?plain=1#L17
- https://github.com/nuxt/nuxt/blob/350788419c414317354de9900cd049fa750456cc/docs/1.getting-started/5.routing.md?plain=1#L25
#2067 was the last change to this regexes, there should also be some ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you've convinced me. I will update regex to allow esacape ] and update docs in that ways (documenting why we choose this method).
const meta = lang | ||
.replace(languageMatches?.[0] ?? '', '') | ||
.replace(highlightTokensMatches?.[0] ?? '', '') | ||
.replace(filenameMatches?.[0] ?? '', '') | ||
.trim() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, it makes this part so much more readable! β€οΈ
Co-authored-by: nobkd <[email protected]>
Co-authored-by: nobkd <[email protected]>
@nobkd we have an issue because a JS string is automatically escape |
We will need to use a double |
See this #2169 (review) We need to use double escaping: There's probably a better escape character, but |
Exactly |
Oh yes, I've not seen this comment! Thanks |
Co-authored-by: nobkd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks guys.
You're awesome β€οΈ
Just to have this in one place, I think this bug was previously introduced with #2067 I think, there was decided, that we could allow |
π Linked issue
β Type of change
π Description
fix #2166
For a code block, if filename was
[...page].vue
, regex was stoping at[...page
. This PR fix it.π Checklist