New eslint warnings in 6.0

[agriffis]

Describe the bug I'm seeing some new eslint warnings as I'm migrating a project from 5.3 to 6.0. These appear to come from Storybook-generated code.

WARNING in ./storybook/preview.js-generated-config-entry.js
Module Warning (from ./node_modules/eslint-loader/dist/cjs.js):

  Line 1:1:     'use strict' is unnecessary inside of modules                                   strict
  Line 11:185:  Function declared in a loop contains unsafe references to variable(s) 'source'  no-loop-func
  Line 11:412:  Function declared in a loop contains unsafe references to variable(s) 'source'  no-loop-func

To Reproduce I don't have a stripped-down reproducer yet. Before going through that effort, I thought it would be good to see if this is a problem other people are already seeing and aware of.

Expected behavior No eslint warnings from Storybook generated code.

System: I run storybook in a docker container, so this reflects that:

$ npx -p @storybook/cli@next sb info

Environment Info:

  System:
    OS: Linux 5.6 Debian GNU/Linux 9 (stretch) 9 (stretch)
    CPU: (12) x64 Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
  Binaries:
    Node: 12.16.1 - /usr/local/bin/node
    Yarn: 1.22.0 - /usr/local/bin/yarn
    npm: 6.13.4 - /usr/local/bin/npm
  npmPackages:
    @storybook/addon-actions: ^6.0.0-beta.12 => 6.0.0-beta.12 
    @storybook/addon-links: ^6.0.0-beta.12 => 6.0.0-beta.12 
    @storybook/addon-viewport: ^6.0.0-beta.12 => 6.0.0-beta.12 
    @storybook/addons: ^6.0.0-beta.12 => 6.0.0-beta.12 
    @storybook/preset-create-react-app: ^3.0.0 => 3.0.0 
    @storybook/react: ^6.0.0-beta.12 => 6.0.0-beta.12 

My storybook/main.js is

const path = require('path')

module.exports = {
  stories: ['../src/**/*.stories.js', '../src/**/stories.js'],

  addons: [
    '@storybook/preset-create-react-app',
    '@storybook/addon-actions',
    '@storybook/addon-links',
    '@storybook/addon-viewport',
    './storybook/addons/admin/register.js',
  ],  managerWebpack: config => {
    config.resolve.modules = [
      ...(config.resolve.modules || []),
      path.resolve(__dirname, '../src'),
    ]
    return config
  },
}

[agriffis]

If I add /* eslint-disable */ to the tops of these files, the warnings go away:

• node_modules/@storybook/core/dist/server/preview/virtualModuleEntry.template.js
• node_modules/@storybook/core/dist/server/preview/virtualModuleStory.template.js

Obviously not a proper fix, just tracking down where these are coming from.

[agriffis]

Still a problem in 6.0.0-beta.16

[stale[bot]]

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!

[agriffis]

Still a problem

[stale[bot]]

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!

[shilman]

How is your project set up? Do you have a repro repo?

[jookshub]

I've ran into the same problem.

build-storybook does not respect .eslintrc.json and I have found no sufficient information how to configure it, so that it will respect it or an .eslintignore

See my repo https://github.com/jookshub/storybook_cra_tsx/tree/feature/demo-eslint-storybook-issues

Compare running build-storybook vs. npm run lint

[jookshub]

After some experimentation I got Storybook 6.0.0-rc.3 to respect a given .eslintrc.json or .eslintignore or the eslintConfig section of the package.json.

Key to this was to use EXTEND_ESLINT=true build-storybook, so that CRA would set useEslintrc: true - see here.

Additionally it was necessary to re-trigger the eslint-loader via webpackFinal in the .storybook/main.js config as such:

webpackFinal: async (config) => {
    config.module.rules.push({
                               test: /\.(js|mjs|jsx|ts|tsx)$/,
                               enforce: 'pre',
                               exclude: /node_modules/,
                               loader: 'eslint-loader'
                             });
    return config;
  }

This last step seems unnecessary, as @storybook could trigger it as a default in case EXTEND_ESLINT=true, it might be even a good default to set it to true by @storybook if an eslint-Config is present in the current project.

There might be a good reason why all of this works as it does, however, that reasoning and the solution require further documentation - but for now, this solution is sufficient for me 🙂

ℹ️ This workaround works, with react-scripts: 3.4.1 and an own install of eslint: >6.8.0. Somehow, without an own eslint in package.json this solutions stops working and will result in duplicate lint errors with different console-highlights.

[shilman]

Nice find @jookshub! @mrmckeb should we build this into the CRA preset somehow?

[mrmckeb]

@shilman this is actually required to use a custom ESLint config with CRA today and is documented within CRA. https://github.com/facebook/create-react-app/blob/c87ab79559e98a5dae2cd0b02477c38ff6113e6a/docusaurus/docs/advanced-configuration.md

As the preset loads the CRA config, most of the same settings will apply. I'm a little confused as to why the rule needs to be added in again though, that would need investigation.

@agriffis if you'd like to raise an issue against the preset for Storybook - https://github.com/storybookjs/presets - I can take a look shortly, I think after the next release of CRA (4.0) as it's due very soon and we might want to make some minor changes then anyway.

[agriffis]

@mrmckeb Ah that explains it!

I'm using https://github.com/harrysolovay/rescripts with the env rescript, and that fills the same role as setting EXTEND_ESLINT. I'd forgotten about this function of rescripts. :blush:

I'll file an issue against the preset for investigating why EXTEND_ESLINT doesn't apply properly to the storybook build.

Thanks @jookshub for helping to investigate this!

[agriffis]

In fact it looks like there's already an issue https://github.com/storybookjs/presets/issues/136

[agriffis]

Okay, I have some new info... painfully obtained by hours of small tweaks, purging of node_modules and pulling of hair.

I am using yarn build-storybook -c storybook -- as in, my top storybook dir is (no-dot) storybook instead of (dot) .storybook

I created a super simple repro at https://github.com/agriffis/storybook-eslint-demo

[stale[bot]]

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!

[mattfelten]

I'm experiencing the same exact warning right now and, yes, also using a custom storybook location. Moving my storybook folder to <root>/.storybook removes that warning.

[stale[bot]]

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!

[shilman]

We’re cleaning house! Storybook has changed a lot since this issue was created and we don’t know if it’s still valid. Please open a new issue referencing this one if:

• this is still a problem in SB 7.x
• you can provide a consistent reproduction in 7.x (at https://storybook.new/ or per repro docs).