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

Force NODE_ENV=production for Storybook builds through the CLI #865

Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Nov 30, 2023

It seems this can get set to development when builds are spawned from the Visual Tests addon. There's no reason to build in dev, AFAIK.

I guess technically this might be a breaking change, not sure.

📦 Published PR as canary version: 9.1.1--canary.865.7041382024.0

✨ Test out this PR locally via:

npm install [email protected]
# or 
yarn add [email protected]

@tmeasday tmeasday requested a review from ghengeveld November 30, 2023 02:50
Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this should be the default behavior? If so then I think this change is okay as-is and we should to a major version bump. Otherwise we should make it opt-in via an option (e.g. forceProductionBuild).

@tmeasday
Copy link
Member Author

tmeasday commented Nov 30, 2023

Yes, I do think it should be the default (I'm not aware of any circumstances when you wouldn't want this). I'll leave it to you to do the major merge?

@tmeasday tmeasday added the major Auto: Increment the major version when merged label Nov 30, 2023
@ghengeveld ghengeveld added the release Auto: Create a `latest` release when merged label Dec 1, 2023
@ghengeveld ghengeveld changed the title Force production environment for CLI builds Force NODE_ENV=production for Storybook builds through the CLI Dec 1, 2023
@ghengeveld ghengeveld added this pull request to the merge queue Dec 1, 2023
Merged via the queue into main with commit 7873bb4 Dec 1, 2023
20 of 22 checks passed
@ghengeveld ghengeveld deleted the tom/ap-3897-force-production-environment-in-cli-builds branch December 1, 2023 11:42
@ghengeveld
Copy link
Member

🚀 PR was released in v10.0.0 🚀

@ghengeveld ghengeveld added the released Verdict: This issue/pull request has been released label Dec 1, 2023
@isweetland
Copy link

@ghengeveld @tmeasday

We are using the github action to release stories that include interactions. We set NODE_ENV=test or else we see the following error when calling userEvent.click from @storybook/testing-library

act(...) is not supported in production builds of React.

Is using @storybook/testing-library a circumstance where you wouldn't want to force NODE_ENV=production?

@tmeasday
Copy link
Member Author

tmeasday commented Dec 1, 2023

Hi @isweetland thanks for bringing this up.

I wasn't aware of the need to do this. Certainly using testing library is possible with the standard production builds but maybe there some patterns we aren't aware of!

Can you post a snippet of code that triggers this?

@isweetland
Copy link

isweetland commented Dec 1, 2023

Yep! Here you go:

Edit: @storybook/react is v7.5.3, @storybook/testing-library is v0.2.2

import { StoryObj } from '@storybook/react';
import { userEvent, within } from '@storybook/testing-library';

const meta = {
  title: 'Waitlist',
};

export default meta;

export const WithWaitlist: StoryObj = {
  render: () => {
    return <button>Join Waitlist</button>;
  },
  play: async ({ canvasElement }) => {
    const canvas = within(canvasElement);

    await userEvent.click(
      await canvas.findByRole('button', { name: 'Join Waitlist' }),
    );
  },
};
Screen Shot 2023-12-01 at 4 57 07 PM

@tmeasday
Copy link
Member Author

tmeasday commented Dec 1, 2023

Thanks @isweetland, I'm not seeing the behaviour though. Can you give a couple more details (or post a SB reproduction)?

What I did:

  1. Created a react-vite Storybook sandbox
  2. Added your story above
  3. Built it with yarn build-storybook
  4. Ran a http server
  5. Visited the story, checked the interactions worked:
image

@isweetland
Copy link

isweetland commented Dec 2, 2023

@tmeasday yes, I just built in a fresh sandbox and observed the same. I think I might have found the issue thanks to this comment. It seems like we're importing @testing-library/react (and other @testing-library/* libs) into our stories thanks to a wrapper component exported from a testing utils folder.

Being mindful about these imports seems to fix this! We'll resolve the problem and then update to v10.0.0

Thanks!

@tmeasday
Copy link
Member Author

tmeasday commented Dec 3, 2023

OK cool! Well we can always add the ability to force a different node env somehow, but it seems like something that's better avoided entirely.

@brett-east
Copy link

Sorry, I'm not sure what the correct forum to discuss this is. What is the reasoning behind saying "There's no reason to build in dev"? We always build our chromatic builds in dev, as we don't need to use our production webpack settings for storybook. Forcing us to build in production also throws errors for us, because it looks like we have devDependencies in our webpack build and they're missing when storybook tries to run - I'm not sure why storybook doesn't install all devDependencies.

@tmeasday
Copy link
Member Author

tmeasday commented Dec 4, 2023

@brett-east this seems like a fine place to discuss it.

What is the reasoning behind saying "There's no reason to build in dev"?

You missed the key "AFAIK" from the end of that :) Just that in my experience folks are building their Storybooks to be deployed to the internet so the same production considerations (like minimizations etc) apply. I wasn't aware of any situations where you wouldn't want that.

We always build our chromatic builds in dev, as we don't need to use our production webpack settings for storybook.

So how do you go about that? Do you set the env var in the build-storybook script? The chromatic CLI command? Or do you build SB separately?

Just trying to figure out what the best way for us to supply an escape hatch here is.

As an aside in the short term, you can just build your Storybook in a separate step to Chromatic and pass it with the --storybook-build-dir flag as a workaround.

Forcing us to build in production also throws errors for us, because it looks like we have devDependencies in our webpack build and they're missing when storybook tries to run - I'm not sure why storybook doesn't install all devDependencies.

Sounds like maybe your CI script is doing something odd there in terms of the dependencies it installs before running Chromatic?

@AmirL
Copy link

AmirL commented Dec 15, 2023

Hi @tmeasday

I've spend a few hours today trying to understand why I start getting errors on Chromatic. Thanks to Carmen from the support, I got here.

We always build Storybook in the dev mode, and push it to Chromatic:

      - name: Publish to Chromatic
        uses: chromaui/action@v1
        with:
          projectToken: ${{ secrets.CHROMATIC_PROJECT_TOKEN }}
          onlyChanged: true
          exitZeroOnChanges: true
          exitOnceUploaded: true
          workingDir: apps/front-customers
          storybookBuildDir: storybook-static

We use Relay test library to mock GraphQl requests, and obviously, it doesn't work in the production mode. (Who ever would want to keep it enabled in the prod, right?).

Now I have a strange situation with Chromatic. My builds don't work, and no screenshots and etc. Although my static versions of Storybooks work well (on Chromatic also).

How can I fix it? Can you enable dev mode somehow? Do I need to create a new issue for that, or it's possible to reopen this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Auto: Increment the major version when merged release Auto: Create a `latest` release when merged released Verdict: This issue/pull request has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants