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

Storybook warns when function named add is called in story file. #12208

Closed
heyimalex opened this issue Aug 24, 2020 · 16 comments
Closed

Storybook warns when function named add is called in story file. #12208

heyimalex opened this issue Aug 24, 2020 · 16 comments

Comments

@heyimalex
Copy link
Contributor

Storybook issues a warning when any function named add is called with a non-string first arg. The warning happens at build time, and doesn't trigger if the add function isn't being called as a property (ie must be blah.add(1,2)).

The warning generated:

WARNING in ./src/stories/Button.stories.tsx
Module build failed (from ./node_modules/@storybook/source-loader/dist/index.js):
TypeError: string.toLowerCase is not a function
    at sanitize (/Users/alexander.guerra/sbtest/node_modules/@storybook/csf/dist/index.js:27:17)
    at handleADD (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/abstract-syntax-tree/parse-helpers.js:173:15)
    at Controller.enter (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/abstract-syntax-tree/traverse-helpers.js:138:54)
    at Controller.__execute (/Users/alexander.guerra/sbtest/node_modules/estraverse/estraverse.js:330:31)
    at Controller.traverse (/Users/alexander.guerra/sbtest/node_modules/estraverse/estraverse.js:434:28)
    at Object.traverse (/Users/alexander.guerra/sbtest/node_modules/estraverse/estraverse.js:646:27)
    at findAddsMap (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/abstract-syntax-tree/traverse-helpers.js:132:14)
    at generateAddsMap (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/abstract-syntax-tree/generate-helpers.js:209:43)
    at generateStoriesLocationsMap (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/abstract-syntax-tree/generate-helpers.js:213:22)
    at inject (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/abstract-syntax-tree/inject-decorator.js:65:66)
    at readAsObject (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/dependencies-lookup/readAsObject.js:26:48)
    at readStory (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/dependencies-lookup/readAsObject.js:42:10)
    at Object.transform (/Users/alexander.guerra/sbtest/node_modules/@storybook/source-loader/dist/build.js:13:38)
 @ \.)(?=.)[^/]*?\.stories\.(js|jsx|ts|tsx))$ (./src sync ^\.(?:(?:^|\/|(?:(?:(?!(?:^|\/)\.).)*?)\/)(?!\.)(?=.)[^/]*?\.stories\.(js|jsx|ts|tsx))$) ./stories/Button.stories.tsx
 @ ./.storybook/generated-stories-entry.js
 @ multi ./node_modules/@storybook/core/dist/server/common/polyfills.js ./node_modules/@storybook/core/dist/server/preview/globals.js ./.storybook/storybook-init-framework-entry.js ./node_modules/@storybook/addon-docs/dist/frameworks/common/config.js-generated-other-entry.js ./node_modules/@storybook/addon-docs/dist/frameworks/react/config.js-generated-other-entry.js ./node_modules/@storybook/addon-links/dist/preset/addDecorator.js-generated-other-entry.js ./node_modules/@storybook/addon-actions/dist/preset/addDecorator.js-generated-other-entry.js ./node_modules/@storybook/addon-actions/dist/preset/addArgs.js-generated-other-entry.js ./node_modules/@storybook/addon-backgrounds/dist/preset/defaultParameters.js-generated-other-entry.js ./.storybook/preview.js-generated-config-entry.js ./.storybook/generated-stories-entry.js ./node_modules/webpack-hot-middleware/client.js?reload=true&quiet=false&noInfo=undefined

To Reproduce

  1. create-react-app + sb init a basic project
  2. Go into one of the auto-generated stories and add this:
const blah = { add: (a, b) => a + b };
blah.add(1, 2);
  1. run yarn storybook

The warning message should show up in the console during build. The warning is also issued without including the first line (so the second line is what actually triggers it), but I included it just to prove that it fails on valid code.

Expected behavior

There shouldn't be a build time warning; this is valid code.

System:

Environment Info:

  System:
    OS: macOS 10.15.5
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
  Binaries:
    Node: 14.7.0 - /usr/local/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.14.7 - /usr/local/bin/npm
  Browsers:
    Chrome: 84.0.4147.135
    Safari: 13.1.1
  npmPackages:
    @storybook/addon-actions: ^6.0.16 => 6.0.16
    @storybook/addon-essentials: ^6.0.16 => 6.0.16
    @storybook/addon-links: ^6.0.16 => 6.0.16
    @storybook/node-logger: ^6.0.16 => 6.0.16
    @storybook/preset-create-react-app: ^3.1.4 => 3.1.4
    @storybook/react: ^6.0.16 => 6.0.16

Additional context

From my reading of the warning (handleADD in @storybook/source-loader/dist/abstract-syntax-tree/parse-helpers.js), it looks like this is caused by some special parsing of storybook files. My guess is the parser is trying to pull out storiesOf(...).add( calls. The error message, string.toLowerCase is not a function, jives with this: storybook's add takes a string as the first param, but I pass in a number. To drive this home, the build doesn't issue this warning if you change the line added to blah.add("1", "2").

Workaround

Ran into this because we were defining a constant using moment.add(...). Changing the property access to use bracket notation (ie moment["add"](...) fixed the issue.

@heyimalex heyimalex changed the title Storybook warns when any function named add is called in story file with non-string first arg. Storybook warns when function named add is called in story file. Aug 24, 2020
@shilman
Copy link
Member

shilman commented Aug 24, 2020

That's exactly right. I think fix here is to create a source-loader replacement that:

  • is optimized, cleaned, tested
  • only handles CSF and doesn't worry about storiesOf format

@stale
Copy link

stale bot commented Sep 14, 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 Sep 14, 2020
@jaredcwhite
Copy link

jaredcwhite commented Sep 22, 2020

This is still very much a bug! We're having problems with moment.add in some of our stories, even with v6.0.21. Same error message: string.toLowerCase is not a function

@stale stale bot removed the inactive label Sep 22, 2020
@jaredcwhite
Copy link

Seems like a workaround is to assign the number to a variable ahead of time, e.g.:

const days = 10
dateString = moment().add(days, "days").format()

@bokub
Copy link

bokub commented Oct 18, 2021

I encountered the same problem with dayjs.

My workaround was to use substract instead of add:

- dayjs().add(1, 'day').format('YYYY-MM-DD')
+ dayjs().subtract(-1, 'day').format('YYYY-MM-DD')

@pbjorklund
Copy link

Same issue (and giving a var as first arg to add workaround) still persists today with

addons: [
    "@storybook/addon-essentials",
    "@storybook/addon-knobs",
    "@storybook/addon-links/register",
    "storybook-addon-designs/register",
    "@storybook/preset-create-react-app",
    "@storybook/addon-storysource",
],
core: {
    builder: 'webpack5',
  },

@lizthegrey
Copy link

I can reproduce this problem with the webpack5 builder at 6.4.18 as well. Suspecting the fix in #11920 didn't quite fully do it.

@msakrejda
Copy link
Contributor

I just hit this as well (upgrading from 6.4.0-beta.11 to 6.4.19). I'm using storybook-builder-vite for what it's worth. @jaredcwhite's workaround worked for me (thanks!).

@apedroferreira
Copy link

same here with dayjs(...).add(...)

@robbeman
Copy link

robbeman commented Jun 1, 2022

A silly workaround 🙃

dayjs().subtract(-1, 'day');

@rhuanbarreto
Copy link

Discovered the same here with dayjs().add(). Storybook v6.5.3

@acherkashin
Copy link

I face the same issue with moment: moment().add(days, 'days').

module.exports = {
  webpackFinal: config => {
    return {
      ...config,
      resolve: {
        ...config.resolve,
        modules: [...(config.resolve.modules || []), path.resolve('./src')],
      },
    };
  },
  stories: ['../src/**/*.stories.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'],
  addons: [
    '@storybook/addon-links',
    '@storybook/addon-essentials',
    '@storybook/addon-interactions',
    '@storybook/preset-create-react-app',
  ],
  framework: '@storybook/react',
  core: {
    builder: '@storybook/builder-webpack5',
  },
};

node version - 16.14.0

packages:

    "@storybook/addon-actions": "^6.5.9",
    "@storybook/addon-console": "^1.2.3",
    "@storybook/addon-essentials": "^6.5.9",
    "@storybook/addon-interactions": "^6.5.9",
    "@storybook/addon-links": "^6.5.9",
    "@storybook/builder-webpack5": "^6.5.9",
    "@storybook/manager-webpack5": "^6.5.9",
    "@storybook/node-logger": "^6.5.9",
    "@storybook/preset-create-react-app": "^4.1.2",
    "@storybook/react": "^6.5.9",
    "@storybook/testing-library": "^0.0.13",

btw, previously I used ejected CRA and manually merged ejected CRA webpack config with storybook config and didn't have this issue. Currently we've forked CRA and use @storybook/preset-create-react-app and got this issue.

@shilman
Copy link
Member

shilman commented Sep 27, 2022

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-alpha.34 containing PR #18930 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb upgrade --prerelease

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

@shilman shilman closed this as completed Sep 27, 2022
@vladstarikov88
Copy link

@shilman is it possible to merge this PR to LTS version? 6.15 for example

@Cilia94
Copy link

Cilia94 commented Jan 19, 2023

Seems like a workaround is to assign the number to a variable ahead of time, e.g.:

const days = 10
dateString = moment().add(days, "days").format()

I found another solution based on your reply! Instead of having to define an additional variable to pass down, you can use a template literal:

dateString = moment().add(`${10}`, "days").format()

@eranimo
Copy link

eranimo commented Jan 27, 2023

This needs to be backported to v6

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

No branches or pull requests