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 storybook config #7988

Closed
wants to merge 1 commit into from

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Mar 3, 2022

@roryabraham

Details

Update storybook config after changes to common webpack configuration

Fixed Issues

$ N/A

Tests

  • Verify npm storybook-build creates a bundle that can be served (outputted at dist/docs)
  • Verify npm run storybook starts the local storybook dev server

PR Review Checklist

Contributor (PR Author) Checklist

  • I made sure to pull main before submitting my PR for review
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I ran the tests & they passed on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
  • I followed the guidelines as stated in the Review Guidelines

PR Reviewer Checklist

  • I verified the Author pulled main before submitting the PR
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I verified tests pass on all platforms & I tested again on all platforms
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines

QA Steps

  1. Go to https://staging.new.expensify.com/docs/index.html
  2. The documentation page should load.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2022-03-03 at 18 57 28

Mobile Web

Desktop

iOS

Android

@kidroca kidroca requested a review from a team as a code owner March 3, 2022 16:58
@MelvinBot MelvinBot requested review from francoisl and removed request for a team March 3, 2022 16:59
Copy link
Contributor Author

@kidroca kidroca left a comment

Choose a reason for hiding this comment

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

Added some notes

'react-native$': 'react-native-web',
'@react-native-community/netinfo': path.resolve(__dirname, '../__mocks__/@react-native-community/netinfo.js'),
};
const webConfig = getCommonConfig({envFile: '.env.production'});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct path is .env.production (vs ../.env.production) - tested this by using stuff from CONFIG in stories and it would give the prod values

Comment on lines -13 to -17
config.resolve.alias = {
'react-native-config': 'react-web-config',
'react-native$': 'react-native-web',
'@react-native-community/netinfo': path.resolve(__dirname, '../__mocks__/@react-native-community/netinfo.js'),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to overwrite config.resolve.alias, but if you check it you will see a lot of storybook related aliases in there. Instead of overwriting we should merge our changes in

config.resolve.alias = {
'react-native-config': 'react-web-config',
'react-native$': 'react-native-web',
'@react-native-community/netinfo': path.resolve(__dirname, '../__mocks__/@react-native-community/netinfo.js'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why we used the a netinfo mock here, I've removed it and didn't see any problems

tagging @marcaaron since I saw you on the blame

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not remember, sorry. If it works without it then 🚀

@francoisl
Copy link
Contributor

Hi, is there a GH issue or Slack discussion somewhere where I can get some background context on this change? Thanks!

@kidroca
Copy link
Contributor Author

kidroca commented Mar 3, 2022

It's related to a config regression found here: #7744 (comment)
Rory submitted this PR #7955 first, but I think the path to .env is not working, and find a way to shorten the storybook config

@kidroca kidroca changed the title [NO QA] Fix storybook config Fix storybook config Mar 3, 2022
.storybook/webpack.config.js Show resolved Hide resolved
const babelRulesIndex = _.findIndex(custom.module.rules, rule => rule.loader === 'babel-loader');
const babelRule = custom.module.rules[babelRulesIndex];
const babelRulesIndex = _.findIndex(webConfig.module.rules, rule => rule.loader === 'babel-loader');
const babelRule = webConfig.module.rules[babelRulesIndex];
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we can simplify this a bit:

const babelRule = _.find(webConfig.module.rules, rule => rule.loader === 'babel-loader');

@roryabraham
Copy link
Contributor

Sorry, @kidroca is there a bug report somewhere? I'm not sure what this is actually fixing, the storybook site seems to be working okay for me: https://staging.new.expensify.com/docs/index.html.

I know you mentioned that the environment file is wrong (and I'm 👍 to fix it in this PR), but I just want to make sure I know if there's something specific that is broken that we should be testing here.

@kidroca
Copy link
Contributor Author

kidroca commented Mar 4, 2022

Sorry, @kidroca is there a bug report somewhere? I'm not sure what this is actually fixing, the storybook site seems to be working okay for me: https://staging.new.expensify.com/docs/index.html.

You brought storybook as a regression here: #7744 (comment)
I've prepared a PR but saw you already submitted and merged another

Decided to post this today as it reduces the overall config

@kidroca
Copy link
Contributor Author

kidroca commented Sep 16, 2022

Close old PR

@kidroca kidroca closed this Sep 16, 2022
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.

None yet

4 participants