-
Notifications
You must be signed in to change notification settings - Fork 6
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(storybook-generator): unescaped md content may break storybook #713
fix(storybook-generator): unescaped md content may break storybook #713
Conversation
🚀 Latest successful build of the PR deployed here. 🚀 |
@@ -111,9 +111,14 @@ function buildStoryJs(story, html) { | |||
title: '${story.title}' | |||
}; | |||
|
|||
export const ${story.name} = () => \`${html}\`; | |||
export const ${story.name} = () => String.raw\`${ |
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.
Can you please move this to the applyCommonTransformations
method (and then there is also no need in String.raw
here, so that this method stays the same and that method is getting enhanced?
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, but note that I have purposely chosen to include this transformation in buildStoryJs
since all the escaping done (String.raw
and +
concatenations) are exclusively aimed at building the JS module in a safe manner which should enable the html content to land unmodified in the resulting story.
We may also want to keep String.raw
, as it will preserve the original backslashes coming from the HTML content (after transformation from md). Otherwise JS will escape these sequences, leading to unexpected result (the MD author should remain oblivious to the JS trickery happening).
LMK if you still want me to modify something
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.
Okay, BTW, if the slashes are preserved, why do you need to replace/duplicate them explicitly?
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.
There is the rare case in which backslash will be the last character before the closing string tick, which will result in an exception. That's the only reason for .replace(/(\\+)`/g, '` + "$1$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.
can this happen in HTML? I don't care to leave it but looks really obsolete case to me...
up to you, I'll approve, if you'll see it reasonable to remove, will reapprove :)
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, unfortunately.. If your source md is something like
Hello World\\
, the "\\" sequence will be converted to "\" by the markdown processor, which will leave a "\" in the html string. When this string will be converted to a module, there will be a string literal ending with a backslash..
Truly rare, but nice to have 🙂
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.
As I said, I can't imagine this case, not even rare, that someone would end MD file like this (and BTW, this case is covered by the top level String.raw that you've added, no?). But this discussion looses it, so you can proceed with the PR from my POV.
Kudos, SonarCloud Quality Gate passed! |
Documented here.