defining process as true breaks usage of libraries looking for environment variables

[travi]

Describe the bug

process is defined as true using the webpack define plugin. true does not match the expectation that process is an object.

this results in libraries, even when attempting to be defensive about the presence of process, attempting to perform object actions against a boolean and therefore throwing errors at runtime.

the specific example that is giving me trouble is with the browser version of debug.

To Reproduce

Steps to reproduce the behavior:

1. import debug into one of your stories
2. navigate to that story within storybook
3. See error in the browser console

Expected behavior

process should either be left as undefined and expected to be added by projects that need it or at least defined using a shape that matches the expected contract. please also see this comment from a maintainer of debug about the contract of process

error

the error thrown in the case of `debug`

TypeError: Cannot use 'in' operator to search for 'env' in true
    at Function.load (browser.js?34eb:221)
    at setup (common.js?dc90:261)
    at eval (browser.js?34eb:250)
    at Object../node_modules/debug/src/browser.js (vendors~main.90358a9d4fdec3ed3967.bundle.js:75360)
    at __webpack_require__ (runtime~main.90358a9d4fdec3ed3967.bundle.js:782)
    at fn (runtime~main.90358a9d4fdec3ed3967.bundle.js:150)
    at eval (any.es.js?d9f8:39)
    at Module../node_modules/@travi/any/lib/any.es.js (vendors~main.90358a9d4fdec3ed3967.bundle.js:68352)
    at __webpack_require__ (runtime~main.90358a9d4fdec3ed3967.bundle.js:782)
    at fn (runtime~main.90358a9d4fdec3ed3967.bundle.js:150)

Code snippets

https://github.com/visionmedia/debug/blob/5c7c61dc0df0db4eb5de25707d8cd1b9be1add4f/src/browser.js#L216

System:

• OS: MacOS
• Device: Macbook Pro 2016
• Browser: chrome
• Framework: react
• Addons: actions, info, links
• Version: [e.g. 4.0.0] found initially on v5.1.0-alpha.40, but have confirmed that it still exists in v5.1.0-beta.0

Additional context

i did add a comment yesterday evening against the commit that appeared to add this definition. its it can be bad form to comment against closed issues, i figured a more formal issue could be better. please don't see the double filing as any additional pressure from me. i'm just trying to follow up to get you the info that aligns with your normal process.

[lewisl9029]

I was just about to report the exact same issue. In my case our project used https://github.com/nygardk/react-share, which had debug as a transitive dependency.

This also appears to overwrite whatever was set previously as process.env in the previous DefinePlugin invocation above, which also feels problematic: https://github.com/storybooks/storybook/pull/6553/files#diff-87391fa0641e90443c27a7173650c0dbR58

[travi]

This also appears to overwrite whatever was set previously as process.env in the previous DefinePlugin invocation above

thats a good point that i forgot to address in my initial issue. i did attempt to use the define plugin to override what was set as process by storybook, but i was unsuccessful. i'm not sure if there is a way to use the define plugin multiple times or not, but i was unsuccessful in my attempts.

[lewisl9029]

Here's the workaround I put in place for now (requires full control mode):

module.exports = ({ config, mode }) => {
  const plugins = [
    ...config.plugins
      // remove instances of DefinePlugin to work around:
      // https://github.com/storybooks/storybook/issues/6763
      .filter((plugin) => !plugin.definitions),
    // replace the default DefinePlugin instance with a custom one that tries to
    // preserve everything except for `process: true`
    new webpack.DefinePlugin({
      "process.env": {
        NODE_ENV: JSON.stringify(mode.toLowerCase()),
        NODE_PATH: JSON.stringify(""),
        PUBLIC_URL: JSON.stringify("."),
      },
      NODE_ENV: JSON.stringify(mode.toLowerCase()),
    }),
  ];

  return {
    ...config,
    plugins,
  };
};

[shilman]

@libetl Didn't you run into this bug and fix it? Can you issue a small PR for that?

[libetl]

Exactly this bug so I made a workaround in poc/addon-editor branch

[libetl]

The above snippet looks good, I also suggest to leave a value for the process.browser boolean

[shilman]

@libetl Can you make a PR onto next to fix the bug so we don't need a workaround at all?

[libetl]

Will do

[libetl]

pull request is now opened

[travi]

since i saw that this got merged, i decided to give it a try again and found that i was still presented with the same error. it took me a bit to realize that i needed to use the debug dist-tag instead of next, but these are my results with 5.2.0-alpha.18

running with the --debug-webpack flag, i see that the "Manager webpack config" now looks like a config similar to what i would expect:

...
DefinePlugin { definitions: {} },
     DefinePlugin {
       definitions:
        { process:
           { browser: true,
             env:
              { NODE_ENV: '"development"', NODE_PATH: '""', PUBLIC_URL: '""' } },
          NODE_ENV: '"development"',
          DOCS_MODE: false } }
...

but the "Preview webpack config" (i didnt realize that two independent configs needed to be changed when i opened this issue) still shows a problematic config:

...
DefinePlugin {
       definitions:
        { 'process.env':
           { NODE_ENV: '"development"', NODE_PATH: '""', PUBLIC_URL: '"."' },
          process: 'true',
          NODE_ENV: '"development"' } },
...
DefinePlugin { definitions: {} }
...

does this mean that both need this change applied?

[shilman]

@travi You should be able to get this on the next dist tag. The debug tag includes the stuff from the next tag, but it's main purpose is as a release channel for SB Docs, and it's not intended to be stable. Does the latest version on next (5.1.0-rc.2?) still not work for you? Do we need to apply the change to both manager & webpack configs?

[travi]

Do we need to apply the change to both manager & webpack configs?

from what i can tell, the change is more necessary on the preview config than on the manager config. i was able to apply the workaround from above to get past it for now, but the change that was merged does not fully resolve the issue for me.

[shilman]

@travi Thanks for the quick response. We'll try to get it taken care of soon.

[travi]

thank you 🙏

[shilman]

Fixed in #6946

Released in https://github.com/storybookjs/storybook/releases/tag/v5.1.0-rc.4