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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Mar 29, 2021
Merged

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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2f0c28
fix(storybook): unescaped md content might break storybook compilation
Mar 18, 2021
cc5df0b
fix(storybook): unescaped md content might break storybook compilation
Mar 18, 2021
d5fcb38
Merge branch 'master' into viv-440-unescaped-md-content-may-break-sto…
yinonov Mar 18, 2021
e44ce95
Merge branch 'master' into viv-440-unescaped-md-content-may-break-sto…
gullerya Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .storybook/build-scripts/create-stories-from-md.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ function validateConfig(config) {
function applyCommonTransformations(htmlInput) {
return `
<link rel="stylesheet" href="assets/css/md-stories.css">
${htmlInput.replace(/\$/g, '\\$')}
${htmlInput}
`;
}

Expand All @@ -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 •
edited
Loading

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.

html
.replace(/([`$])/g, '` + "$1" + String.raw\`')
.replace(/(\\+)`/g, '` + "$1$1"')
}\`;
`;


if (story.parameters) {
result += `
${story.name}.parameters = ${JSON.stringify(story.parameters)};
Expand Down Expand Up @@ -154,4 +159,4 @@ function relocateStaticResources(htmlInput, srPaths, srBasePath = '') {
}
}
return htmlOutput;
}
}