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

fix(storybook-generator): unescaped md content may break storybook #713

Merged
merged 4 commits into from
Mar 29, 2021

Conversation

tveinfeld
Copy link
Contributor

Documented here.

@tveinfeld tveinfeld requested a review from gullerya March 18, 2021 13:31
@github-actions
Copy link

🚀

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\`${
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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"').

Copy link
Contributor

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

Copy link
Contributor Author

@tveinfeld tveinfeld Mar 29, 2021

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 🙂

Copy link
Contributor

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.

@sonarcloud
Copy link

sonarcloud bot commented Mar 29, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@tveinfeld tveinfeld merged commit 5f24b81 into master Mar 29, 2021
@tveinfeld tveinfeld deleted the viv-440-unescaped-md-content-may-break-storybook branch March 29, 2021 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants