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

Syntax Highlighter should import react-syntax-highlighter from CommonJS dist instead of ESModules #9279

Closed
Rikpat opened this issue Dec 31, 2019 · 27 comments · Fixed by #9292

Comments

@Rikpat
Copy link
Contributor

Rikpat commented Dec 31, 2019

Is your feature request related to a problem? Please describe.
I have a problem when using Storyshots (It may be a jest misconfiguration, since I'm not really proficient in that, but I've tried everything to fix it), that syntaxhighlighter imports react-syntax-highlighter from it's esm distribution which creates an error for me

({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import jsx from "refractor/lang/jsx.js";
                                                                                             ^^^^^^

    SyntaxError: Cannot use import statement outside a module

Describe the solution you'd like
I would like to import react-syntax-highlighter from their cjs dist, since syntaxhighlighter component itself is written in commonjs. This fixed my issue locally.

Current imports:

var _jsx = _interopRequireDefault(require("react-syntax-highlighter/dist/esm/languages/prism/jsx"));

var _bash = _interopRequireDefault(require("react-syntax-highlighter/dist/esm/languages/prism/bash"));

var _css = _interopRequireDefault(require("react-syntax-highlighter/dist/esm/languages/prism/css"));

var _markup = _interopRequireDefault(require("react-syntax-highlighter/dist/esm/languages/prism/markup"));

var _tsx = _interopRequireDefault(require("react-syntax-highlighter/dist/esm/languages/prism/tsx"));

var _typescript = _interopRequireDefault(require("react-syntax-highlighter/dist/esm/languages/prism/typescript"));

fixed imports:

var _jsx = _interopRequireDefault(require("react-syntax-highlighter/dist/cjs/languages/prism/jsx"));

var _bash = _interopRequireDefault(require("react-syntax-highlighter/dist/cjs/languages/prism/bash"));

var _css = _interopRequireDefault(require("react-syntax-highlighter/dist/cjs/languages/prism/css"));

var _markup = _interopRequireDefault(require("react-syntax-highlighter/dist/cjs/languages/prism/markup"));

var _tsx = _interopRequireDefault(require("react-syntax-highlighter/dist/cjs/languages/prism/tsx"));

var _typescript = _interopRequireDefault(require("react-syntax-highlighter/dist/cjs/languages/prism/typescript"));

Describe alternatives you've considered
I've been trying to fix the jest config for 2 days now, if you can suggest me how to fix this without the need to change these imports, please do.

Are you able to assist bring the feature to reality?
yes, I can create a PR right away

@shilman
Copy link
Member

shilman commented Jan 1, 2020

@Hypnosphi mind taking a look?

@Hypnosphi
Copy link
Member

Changing imports to cjs makes sense to me, please open a PR

@Rikpat as a temporary workaround, you can try adding following field to your jest.config.js:

transformIgnorePatterns: ['node_modules/(?!react-syntax-highlighter)']

@Rikpat
Copy link
Contributor Author

Rikpat commented Jan 1, 2020

I've made the changes, I only need to verify whether I should keep the requires in as consts like this

const jsx = require('react-syntax-highlighter/dist/cjs/languages/prism/jsx').default;
const bash = require('react-syntax-highlighter/dist/cjs/languages/prism/bash').default;
const css = require('react-syntax-highlighter/dist/cjs/languages/prism/css').default;
const html = require('react-syntax-highlighter/dist/cjs/languages/prism/markup').default;
const tsx = require('react-syntax-highlighter/dist/cjs/languages/prism/tsx').default;
const typescript = require('react-syntax-highlighter/dist/cjs/languages/prism/typescript').default;

ReactSyntaxHighlighter.registerLanguage('jsx', jsx);
ReactSyntaxHighlighter.registerLanguage('bash', bash);
ReactSyntaxHighlighter.registerLanguage('css', css);
ReactSyntaxHighlighter.registerLanguage('html', html);
ReactSyntaxHighlighter.registerLanguage('tsx', tsx);
ReactSyntaxHighlighter.registerLanguage('typescript', typescript);

or if I can inline them since they are only used in single function like this

ReactSyntaxHighlighter.registerLanguage(
  'jsx',
  require('react-syntax-highlighter/dist/cjs/languages/prism/jsx').default
);
ReactSyntaxHighlighter.registerLanguage(
  'bash',
  require('react-syntax-highlighter/dist/cjs/languages/prism/bash').default
);
ReactSyntaxHighlighter.registerLanguage(
  'css',
  require('react-syntax-highlighter/dist/cjs/languages/prism/css').default
);
ReactSyntaxHighlighter.registerLanguage(
  'html',
  require('react-syntax-highlighter/dist/cjs/languages/prism/markup').default
);
ReactSyntaxHighlighter.registerLanguage(
  'tsx',
  require('react-syntax-highlighter/dist/cjs/languages/prism/tsx').default
);
ReactSyntaxHighlighter.registerLanguage(
  'typescript',
  require('react-syntax-highlighter/dist/cjs/languages/prism/typescript').default
);

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 1, 2020

@Rikpat please keep the import statements, only change the paths:

import jsx from 'react-syntax-highlighter/dist/cjs/languages/prism/jsx';
import bash from 'react-syntax-highlighter/dist/cjs/languages/prism/bash';
import css from 'react-syntax-highlighter/dist/cjs/languages/prism/css';
import html from 'react-syntax-highlighter/dist/cjs/languages/prism/markup';
import tsx from 'react-syntax-highlighter/dist/cjs/languages/prism/tsx';
import typescript from 'react-syntax-highlighter/dist/cjs/languages/prism/typescript';

It's OK to import a CommonJS module from ES (or TS) module

@Rikpat
Copy link
Contributor Author

Rikpat commented Jan 1, 2020

@Hypnosphi Sorry I didn't notice your reply before I finished the changes
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

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 1, 2020

@stale
Copy link

stale bot commented Jan 22, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jan 22, 2020
@stale stale bot removed the inactive label Jan 23, 2020
@alexandrzavalii
Copy link

@Rikpat @Hypnosphi Hi! any updates on the PR?

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 30, 2020

@alexandrzavalii #9292 (comment)

open another with just the ESM => CJS change

No one did it yet AFAIK, so you can

@twhoff
Copy link

twhoff commented Feb 4, 2020

@Hypnosphi, the workaround you suggested doesn't seem to work for me (looks like react-syntax-highlighter is required for some reason).

Is there some way to modify the path from ESM to CJS using the config?

@Hypnosphi
Copy link
Member

There isn't AFAIK

@mweber-ak
Copy link

` ({"Object.":function(module,exports,require,__dirname,__filename,global,jest){import jsx from "refractor/lang/jsx.js";

^^^
SyntaxError: Unexpected identifier
`

I am also receiving this. Any update on a fix?

@kimwilson
Copy link

Also receiving the same error as @weber93 using storybook/storyshots with jest (default nextjs config)

@alexandrzavalii
Copy link

alexandrzavalii commented Feb 6, 2020

@Hypnosphi
Not sure if I understood you correctly, but here is the PR https://github.com/storybookjs/storybook/pull/9780/files

@shilman
Copy link
Member

shilman commented Feb 7, 2020

Olé!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-alpha.7 containing PR #9780 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman
Copy link
Member

shilman commented Feb 8, 2020

Gadzooks!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-alpha.9 containing PR #9795 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

@alexandrzavalii
Copy link

@shilman works for me!
Thanks for merging it so quickly :)

@shilman
Copy link
Member

shilman commented Feb 9, 2020

@alexandrzavalii thanks for the contribution!! you rock!!! 💯

@david-golightly-leapyear

Is this slated to be ported to 5.3? I'm running into this issue there.

@david-golightly-leapyear

Nevermind! I see from the discussion that it is. Thanks!

@shilman
Copy link
Member

shilman commented Mar 11, 2020

Crikey!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-alpha.24 containing PR #9292 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

@neil-morrison44
Copy link

Has there been a non-prerelease version released that fixes this or is it still to come?

@vasilevach
Copy link

Hi guys,

I still get the error, even after updating to v6 everything storybook related:

    "@storybook/addon-actions": "6.0.0-alpha.24",
    "@storybook/addon-info": "^5.3.14",
    "@storybook/addon-links": "6.0.0-alpha.24",
    "@storybook/addon-storyshots": "6.0.0-alpha.24",
    "@storybook/addon-viewport": "6.0.0-alpha.24",
    "@storybook/addons": "6.0.0-alpha.24",
    "@storybook/react": "6.0.0-alpha.24",

Anything I might be missing?

 FAIL  src/components/navigation/navigation.spec.js
  ● Test suite failed to run

    /Users/chris/Documents/Projects/TBS-styleguide/node_modules/react-syntax-highlighter/dist/esm/languages/prism/jsx.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import jsx from "refractor/lang/jsx.js";

@shilman
Copy link
Member

shilman commented Mar 14, 2020

@vasilevach Please remove this line:

"@storybook/addon-info": "^5.3.14",

@shilman
Copy link
Member

shilman commented Mar 14, 2020

@neil-morrison44 coming in the next couple days

@shilman
Copy link
Member

shilman commented Mar 14, 2020

Jeepers creepers!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.17 containing PR #9780 that references this issue. Upgrade today to try it out!

@phoenixeliot
Copy link

phoenixeliot commented Jun 1, 2020

I was still getting this on 6.0.0-beta.19 (and addon-info alpha.2):

[omitted]/node_modules/@storybook/addon-info/node_modules/react-syntax-highlighter/dist/esm/languages/prism/jsx.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import jsx from "refractor/lang/jsx.js";
                                                                                                    ^^^

    SyntaxError: Unexpected identifier

      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:537:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:579:25)
      at Object.<anonymous> (node_modules/@storybook/addon-info/node_modules/@storybook/components/dist/syntaxhighlighter/syntaxhighlighter.js:60:35)

In the end this was the transformIgnorePatterns I needed (adding both @storybook/addon-info and its dependency, react-syntax-highlighter):

    "transformIgnorePatterns": [
      "node_modules/(?!lodash-es|antd|@storybook/addon-info|react-syntax-highlighter)"
    ],

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

Successfully merging a pull request may close this issue.