Changed import of react-syntax-highlighter from esm to cjs

[Rikpat]

Issue: #9279

What I did

Changed imports from ESModules to CommonJS modules for react-syntax-highlighter.
I had to change import to require because CommonJS modules don't have typescript definitions and would fail tests.

I noticed PR #9287 and also included that change, but with CommonJS import and manual type definition. In case this PR gets merged, that PR becomes obsolete.
I had to import props for ReactSyntaxHighlighter so I removed the line that got them directly from the component and used the imported props instead.

How to test

Nothing really changed except for imports and typescript declaration, so I don't think any changes in testing or documentation are necessary

Closes #9279, overrides #9287, and also addresses #9282

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/fpk321pjw
✅ Preview: https://monorepo-git-fork-rikpat-patch-3.storybook.now.sh

[Hypnosphi]

@Rikpat #9279 (comment)

there's a problem with import statement, not CommonJS related, but the package doesn't have type definitions for CommonJS modules, so build and tests fail on no type definition.
I had to import it as require to work around this

Otherwise we would need to create a declaration for all of the imported modules

The existing definitions for esm just type everything as any, so I think you can just all the used imports as declare module to lib/components/src/typings.d.ts:

declare module 'react-syntax-highlighter/dist/cjs/languages/prism/jsx';
...etc

[shilman]

@Rikpat Thanks for the fix!!! It looks like it caused some regressions, however. Can you please take a look at the 3 updated Chromatic snapshots and see if you can figure out what's going on? Thanks!

[Rikpat]

@shilman #9287 (comment)

@LarsDenBakker I think they're all from syntax highlighting. I think we need to support HTML/CSS/JS/TS/MD highlighting out of the box.

I tried adding JS and MD into the highligter but it still has the same problem
It seems the generated ASTs are different for some reason, the css seems to be missing number highlighting and JS something to do with modules, and console object isn't highlighted.

I created a codesandbox to find out why does it do that.
It has a Direct component that imports everything directly from cjs dist, DirectHighlight that uses hljs instead of prism (for comparison only) and FromModule that imports from the module.
Just uncommenting the FromModule import in HighlighterTest fixes the css colors when using the direct component, so it's related to some import in there.

[LarsDenBakker]

@Rikpat thanks for digging into this. The strange thing is that your PR has even more differences than my PR to the syntax highlighting. There is definitely something strange going on, perhaps there is a race condition somewhere?

[Rikpat]

@LarsDenBakker I was trying to fix it by importing and registering jsExtras and cssExtras directly from refractor, it fixed js highlighting, but cssExtras do break css highlighting for some reason. It works in isolated environment, but it doesn't work in storybook.
But it's a fix for both yours and mine PR. react-syntax-highlighter imports refractor in full package, but refractor/core in the light package. That includes js and css by default, but it doesn't include jsExtras and cssExtras like the full refractor. And react-syntax-highlighter doesn't include that in their languages.
I just can't figure out why does it break the css.

[ndelangen]

I'm wondering wether changing to https://github.com/rexxars/react-refractor would help.

[mAAdhaTTah]

If that's the case, then yeah, back to my original suggestion of just changing the esm -> cjs imports in this PR. Then I can look at the regressions introduced by those changes separately.

[shilman]

@mAAdhaTTah Are you proposing I merge this and release on an alpha, and then you will follow up from there? (I'm fine with that plan, just want to confirm)

[mAAdhaTTah]

@shilman I'm proposing the PR be updated so it's back to just changing esm -> cjs, if I understood this correctly:

If we were only to change the imports and keep the full react-syntax-highlighter, the regressions would not happen and the PR could be closed.

Then the regression issue (which sounds like is caused by deep-importing that package) could be looked at separately.

[ndelangen]

Yes it sounds like separating these 2 things out into 2 separate PRs is a good plan.

Perhaps best is to leave this one, and open another with just the ESM => CJS change and if everything checks out release that into a 5.3.x patch

[shilman]

@mAAdhaTTah gotcha. glad i checked. sometimes when you're handling 100+ notifications per day, the words just start running together... 🙈

[mAAdhaTTah]

No worries, it happens. I'll poke at #9287 to see what I can do about the deep imports regression.

[mAAdhaTTah]

Not stale.

[shilman]

@mAAdhaTTah you've seen #9780 and #9795 right? both went out in 6.0-alpha

[mAAdhaTTah]

@shilman No, thanks for pointing that out. Unfortunately, I haven't had the time to look into this, but I'm not sure I'm going to in the near future, so if it's in 6.0, maybe that's fine. Sorry I couldn't be more helpful.

[shilman]

@mAAdhaTTah no worries. just wanted to let you and anybody else on this PR know about that incremental change. pretty sure there's more to do here, but it's all a little over my head.

[ndelangen]

This turned into some cleanup & I added 2 new languages: yml & markdown

[Dashue]

Will this land on 5x or should I try to adopt 6x to have this sorted?
I'm hitting the issue when trying to use material-ui.

Happy to jump to 6x if it's in a good state? Are there any migration docs?

[shilman]

@Dashue 6.x and there are migration instructions in MIGRATION.md. I usually write them up in a post as well before the release goes out.

https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#from-version-53x-to-60x