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

Bump react-syntax-highlighter to 13.2.1 #11838

Merged

Conversation

Naturalclar
Copy link
Contributor

@Naturalclar Naturalclar commented Aug 8, 2020

Issue: #11890

there was new security alert for prismjs, which react-syntax-highlighter was using.

What I did

bumped react-syntax-highlighter to 13.2.1 to address security alert for prismjs.

it's a change in major version, but the only breaking change is dracula theme being changed to darcula

https://github.com/react-syntax-highlighter/react-syntax-highlighter/releases

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

n/a

@stof
Copy link
Contributor

stof commented Aug 11, 2020

it would be great to finish this, merge it and release it.
Btw, would it make sense to backport that to the 5.x version as it fixes a security issue in the dependencies ?

@brayfe
Copy link

brayfe commented Aug 12, 2020

+1 for backporting this fix to the 5.x version. Would love to keep dependencies up to date with Storybook!

@fernandopasik
Copy link
Member

Looks like several CI tasks are failing, would you mind update to latest next branch?

Also react-syntax-highlighter has release a few more times up to 13.4.0.

@fernandopasik
Copy link
Member

Please could you also update it in https://github.com/storybookjs/storybook/blob/next/addons/storysource/package.json#L45

@Naturalclar Naturalclar requested a review from igor-dv as a code owner August 19, 2020 03:27
@Naturalclar
Copy link
Contributor Author

@fernandopasik thanks for the comment!
I've pulled the latest next branch and updated dependency

Copy link
Member

@fernandopasik fernandopasik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, maybe @shilman would like to have a look too.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are definitely some breaking changes to deal with before we can merge. @Naturalclar are you able to access the Chromatic diffs linked below? cc @ndelangen

Reviewing_Build_18423_•_storybookjs_storybook

@ndelangen
Copy link
Member

Seems the syntax parsing works great, the theme changed a bit, but we can fix that. I did some quick checks.

@fernandopasik
Copy link
Member

Sorry guys I didn't noticed this. Yes, there's definitely a theme change.

@ndelangen
Copy link
Member

@fernandopasik would you want to fix this today? We could do it together if you'd like?

@fernandopasik
Copy link
Member

Yes, I'll help on this tomorrow.

@ndelangen
Copy link
Member

@Naturalclar want to pair program with us tomorrow?

@Naturalclar
Copy link
Contributor Author

@ndelangen I'd be happy to join on the pair programming if I'm available :)
What time are you planning on working?

@ndelangen
Copy link
Member

17:30 UTC 20th of August (today) 2020
If you send me a DM on discord I can invite you to the meeting, and we should be able to get this working pretty quickly, I think!

https://discord.gg/7yKH3EU

@fernandopasik fernandopasik self-assigned this Aug 20, 2020
@fernandopasik
Copy link
Member

@Naturalclar we went through the code and seems like there's a new option in react-syntax-highlighter that would make at least the biggest issue away

https://github.com/react-syntax-highlighter/react-syntax-highlighter/releases/tag/13.0.0

I hope you don't mind I pushed the change to the branch. I'm looking at the color issues, seems like the theme got messed by some css classes duplicated from the update somehow.

@Naturalclar
Copy link
Contributor Author

@ndelangen @fernandopasik
Sorry I couldn't join the pair programming (it was 2:30am my time (JST), and I was fast asleep)
I don't mind change being pushed to this branch 👍

@ndelangen
Copy link
Member

@Naturalclar happy to chat directly on discord and find a better time-slot for you, I didn't notice your timezone 🙈

@fernandopasik
Copy link
Member

@ndelangen I've narrowed down the issues down to 2, I've tried a few things but didn't make it better.
Could you review if the remaining space issue could be acceptable as new baseline?

@ndelangen
Copy link
Member

💯 x 💯 👏

@ndelangen ndelangen added this to the 6.1 perf milestone Aug 24, 2020
@fernandopasik
Copy link
Member

@shilman could this one be merged?

@ndelangen
Copy link
Member

It will 💯!!

@shilman wants the ensure 6.0.x is stable first, before merging in 6.x.x changes.

@shilman shilman changed the title bump react-syntax-highlighter to 13.2.1 Bump react-syntax-highlighter to 13.2.1 Aug 31, 2020
@shilman shilman merged commit ef715c5 into storybookjs:next Aug 31, 2020
@pavel-sindelka
Copy link

It breaks Storybook for IE 11 #12399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants