Skip to content

Conversation

@tassoevan
Copy link
Contributor

Proposed changes (including videos or screenshots)

Rewrite some Storybook stories in TypeScript, as an example.

Issue(s)

Steps to test or reproduce

Further comments

It's important to note that Storybook's Webpack configuration is mixing babel-loader and ts-loader. It unnecessarily increases memory usage and prevent us to rewrite a .storybook/main.ts because there are many suppressed TypeScript errors across the code.

@tassoevan tassoevan requested a review from a team June 11, 2021 04:22
- name: Build Storybook to sanity check components
run: npm run build-storybook ; rm -rf ./storybook-static
env:
NODE_OPTIONS: --max_old_space_size=8192
Copy link
Member

Choose a reason for hiding this comment

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

We need all this memory to build the storybook? 4096 isn't enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need it (at least while we don't fix the typings issues I've cited), but this heap size is now set on the npm script. Take a look at "storybook" and "build-storybook" in package.json.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I had a memory issue to run the storybook I was thinking if it's necessary 8192 because I solve using 4096, doesn't affect the review, it's just for curiosity :P

Copy link
Member

@dougfabris dougfabris Jun 11, 2021

Choose a reason for hiding this comment

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

Btw, we still have the issue with cookie and cookie-parser when trying to launch storybook 😞

@ggazzo ggazzo merged commit 20be361 into develop Jun 14, 2021
@ggazzo ggazzo deleted the chore/storybook-ts branch June 14, 2021 04:15
@sampaiodiego sampaiodiego mentioned this pull request Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants