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

Fix dotenv resolution #16067

Closed
shilman opened this issue Sep 15, 2021 · 21 comments
Closed

Fix dotenv resolution #16067

shilman opened this issue Sep 15, 2021 · 21 comments

Comments

@shilman
Copy link
Member

shilman commented Sep 15, 2021

There's a bug in a new NextJS project where assert tries to access process.env and fails due to #15925

Proposed fix:

  • Read in dotenv
  • Merge with STORYBOOK_x
  • Define with the webpack define plugin
  • Define process.env as well for compatibility with NextJS

Workaround: create an empty .env file to get NextJS unstuck

@shilman
Copy link
Member Author

shilman commented Sep 21, 2021

w00t!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-alpha.41 containing PR #16105 that references this issue. Upgrade today to the @next 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 21, 2021
@kylemh
Copy link
Member

kylemh commented Oct 2, 2021

FWIW, updated to latest beta with latest Next and my Storybook won't build!

OperationCode/front-end#1435

edit; The build error seems to be with something about my custom webpack configuration; however, if I remove all webpack customization I'm still seeing process is not defined.

@shilman
Copy link
Member Author

shilman commented Oct 3, 2021

@kylemh haven't had a chance to check out your storybook but i just created a fresh next app and npx sb@next init --builder webpack5 on it and don't see the problem FWIW

@anilanar
Copy link

anilanar commented Oct 4, 2021

@shilman I think this PR broke the case when process.env is used in an arrow function such as:

const getFoo = () => process.env.FOO

which becomes

const getFoo = () => { FOO: "\"whatever\"" }.FOO

which is invalid syntax.

Also I'm not sure about how values are double stringified, but that's another issue.

@kylemh
Copy link
Member

kylemh commented Oct 5, 2021

I've given this another go and just want to outline my steps:

  • Use npx sb upgrade --prerelease
  • Use yarn's resolutions to force webpack@5
  • Installed the new builders (@storybook/builder-webpack5@next & @storybook/manager-webpack5@next)
  • Added core: { builder: 'webpack5' } to my config in .storybook/main.js

Dev server seems to build, but the runtime has an error saying that "process is undefined" (among other errors)

Screen Shot 2021-10-04 at 11 52 54 PM

For added context, I don't have - and have never had - a .env file in this project. I use the DefinePlugin in the Storybook Webpack config override; however, even removing it doesn't prevent the run-time errors.

@shilman
Copy link
Member Author

shilman commented Oct 5, 2021

@kylemh does it change anything if instead of using yarn resolutions, you install webpack@5 as a project dependency? I don't see why it would, but I'm at a loss for why you're still seeing this error.

One idea, can you do:

yarn why @storybook/react 
yarn why @storybook/core-server

And see if there are any old versions lying around?

@kylemh
Copy link
Member

kylemh commented Oct 5, 2021

Screen Shot 2021-10-05 at 3 13 39 PM

Nothing.

And just in case...
Screen Shot 2021-10-05 at 3 14 20 PM

I redid the work in a smaller PR, if it helps: OperationCode/front-end#1507

@kamilogerto2
Copy link

I had same issue with process deps.

For me just mocking with DefinePlugin was not working. In my case I needed to use:

new webpack.ProvidePlugin({
      process: 'process/browser',
}),

and

"devDependencies": {
   ...
    "process": "0.11.10",
}

@kylemh
Copy link
Member

kylemh commented Oct 22, 2021

^ works for me too! @shilman

config.plugins.push(new webpack.ProvidePlugin({ process: 'process/browser' }));

@shilman
Copy link
Member Author

shilman commented Oct 22, 2021

@kylemh do you think this should be included in the default config?

@kylemh
Copy link
Member

kylemh commented Oct 22, 2021

If it doesn't hurt anything! I know this is all complex tooling though... I have no idea this negatively affects other setups!

@shilman shilman reopened this Oct 22, 2021
@anilanar
Copy link

anilanar commented Oct 22, 2021

TL;DR I'd do it.

Webpack 4 already included a process shim. In Webpack 5, Tobias decided to drop all node.js shims in favor of moving those responsibilities to userland.

So in theory, storybook's default webpack config can/should provide all shims and polyfills that were enabled in Webpack 4 builder.

Tobias was kinda stubborn in his stance so he completely omitted the knowledge of how to re-polyfill them in webpack 5 migration guide here.

But if you dig deep enough, you can find this, which is the node polyfills activated automatically in Webpack 4 when target is web.

Obviously, Tobias' decision actually made sense. People tried using node.js specific libraries in the browser and webpack auto-mocked their dependencies and users were confused about where the bug is: in webpack or in the node.js specific library ?

@anilanar
Copy link

A valid question is, if we use a custom webpack config for our storybook builds, can we somehow remove node.js specific shims such as process (or any other)? Iterating through config.plugins and trying to remove them isn't viable, IMO.

@shilman
Copy link
Member Author

shilman commented Nov 19, 2021

Great Caesar's ghost!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-rc.4 containing PR #16725 that references this issue. Upgrade today to the @next 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 Nov 19, 2021
@tylersayshi
Copy link

tylersayshi commented Jan 10, 2022

@shilman is it possible that this issue was reintroduced with v6.4.10? I am getting the process is not defined error on 6.4.10 when I was not getting it on 6.4.9.

image

This is a chunk I have had with each SB version to attempt to resolve this issue. It worked before but no longer works:

webpackFinal: (config) => {
  config.plugins = [
      ...config.plugins,
      new webpack.ProvidePlugin({
        process: 'process/browser',
      }),
    ];

  return config;
}

Edit: I just noticed that this was added to the default config so this chunk seems to have been redundant, but still the error in the browser seems to persist.

@shilman
Copy link
Member Author

shilman commented Jan 10, 2022

@tylerjlawson do you have a reproduction i can look at?

@shilman shilman reopened this Jan 10, 2022
@shilman shilman added the linear label Jan 10, 2022
@tylersayshi
Copy link

tylersayshi commented Jan 10, 2022

@shilman Here is a reproduction: https://github.com/tylerjlawson/sb-issue-repro

I assumed the issue was simpler than it is, but the issue I am having seems to occur specifically when the manager.js file imports a file that references process. You should be able to just install and run storybook to see the issue in the browser console. Let me know if that still doesn't throw the error.

Edit/Correction: I made a second commit to further simplify the reproduction, but the issue occurs whenever process is referenced from the manager.js file.

@shilman
Copy link
Member Author

shilman commented Jan 12, 2022

Zoinks!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.5.0-alpha.15 containing PR #17213 that references this issue. Upgrade today to the @next 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 Jan 12, 2022
@shilman
Copy link
Member Author

shilman commented Jan 15, 2022

Good golly!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.13 containing PR #17213 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb upgrade

@curtvict
Copy link
Contributor

Egad!! I just recently started getting this error in my production builds as well (running v6.5.9):

main.2a50b8f3.iframe.bundle.js:1 Uncaught ReferenceError: process is not defined
    at ./.storybook/preview.js-generated-config-entry.js (main.2a50b8f3.iframe.bundle.js:1:232565)
    at __webpack_require__ (runtime~main.d50f313e.iframe.bundle.js:1:387)
    at __webpack_exec__ (main.2a50b8f3.iframe.bundle.js:1:668989)
    at main.2a50b8f3.iframe.bundle.js:1:670503
    at __webpack_require__.O (runtime~main.d50f313e.iframe.bundle.js:1:1048)
    at main.2a50b8f3.iframe.bundle.js:1:670646
    at webpackJsonpCallback (runtime~main.d50f313e.iframe.bundle.js:1:6536)
    at main.2a50b8f3.iframe.bundle.js:1:59

@curtvict
Copy link
Contributor

Ah, found out this was my own dang fault. In a previous commit in my project I had refactored calls like

const config = {
  whatever: process.env.SOMETHING,
}

into

import { SOMETHING } = process.env;

const config {
  whatever: SOMETHING,
}

Reverting that fixed this error for me. Thanks for coming to my TED talk.

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

6 participants