Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.51k stars 2.86k forks source link

Unable to run storybook locally #51108

Open chiragsalian opened 1 week ago

chiragsalian commented 1 week ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: Reproducible in staging?: Reproducible in production?: If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Slack conversation: https://expensify.slack.com/archives/C02NK2DQWUX/p1727459623902909

Action Performed:

npm run storybook

Expected Result:

storybook should have opened without any issues

Actual Result:

Bunch of errors locally image

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

cc @ishpaul777

Issue OwnerCurrent Issue Owner: @ishpaul777
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 (External)

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Anaslancer commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Unable to run storybook locally

What is the root cause of that problem?

The “cannot access before initialization” error occurs when you try to use a variable before it has been initialized.

What changes do you think we should make in order to solve the problem?

We should declare and initialize the variable before we use it. The solution is to change this part.

let Link, LinkingContext, NavigationContainer, ServerContainer, DarkTheme, DefaultTheme, ThemeProvider, useLinkBuilder, useLinkProps, useLinkTo, useScrollToTop;
Link = realReactNavigation.Link;
LinkingContext = realReactNavigation.LinkingContext;
NavigationContainer = realReactNavigation.NavigationContainer;
ServerContainer = realReactNavigation.ServerContainer;
DarkTheme = realReactNavigation.DarkTheme;
DefaultTheme = realReactNavigation.DefaultTheme;
ThemeProvider = realReactNavigation.ThemeProvider;
useLinkBuilder = realReactNavigation.useLinkBuilder;
useLinkProps = realReactNavigation.useLinkProps;
useLinkTo = realReactNavigation.useLinkTo;
useScrollToTop = realReactNavigation.useScrollToTop;

What alternative solutions did you explore? (Optional)

N/A

Contributor details

Your Expensify account email: anasup1995@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01aff093c9a804b145

melvin-bot[bot] commented 1 week ago

📣 @Anaslancer! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
Anaslancer commented 1 week ago

Screenshot_4

melvin-bot[bot] commented 1 week ago

@trjExpensify, @ishpaul777 Whoops! This issue is 2 days overdue. Let's get this updated quick!

ishpaul777 commented 1 week ago

i'll review proposals soon

ishpaul777 commented 6 days ago

Thanks for your proposal @Anaslancer, But I dont think your proposal identifies the root cause of the problem correctly, As I understand, it seems to be case of circular depency in importing of module @react-navigation/native we are importing the module in its mock itself.

https://github.com/Expensify/App/blob/66cf824036a51183d73dab2cc621901e43fe3196/__mocks__/%40react-navigation/native/index.ts#L7

Anaslancer commented 6 days ago

Thank you, @ishpaul777 We are using jest.requireActual in this file when isJestEnv is true. https://github.com/Expensify/App/blob/66cf824036a51183d73dab2cc621901e43fe3196/__mocks__/%40react-navigation/native/index.ts#L7

jest.requireActual(moduleName) Returns the actual module instead of a mock, bypassing all checks on whether the module should receive a mock implementation or not.

So at the moment there is no error, because realReactNavigation works like object. But when isJestEnv is false, we are just using the declarations. So we are getting the above error I think. @ishpaul777 Please let me know your idea. Thanks

ishpaul777 commented 6 days ago

we need to mock the used componenets from realReactNavigation, i am not sure if this is a best way to do this, open to more ideas...

diff --git a/__mocks__/@react-navigation/native/index.ts b/__mocks__/@react-navigation/native/index.ts
index 5bcafdc1856..1a56174209e 100644
--- a/__mocks__/@react-navigation/native/index.ts
+++ b/__mocks__/@react-navigation/native/index.ts
@@ -16,9 +16,11 @@ const {triggerTransitionEnd, addListener} = isJestEnv
           addListener: () => {},
       };

+const realOrMockedUseNavigation = isJestEnv ? realReactNavigation.useNavigation : {};
+
 const useNavigation = () => ({
-    ...realReactNavigation.useNavigation,
-    navigate: jest.fn(),
+    ...realOrMockedUseNavigation,
+    navigate: isJestEnv ? jest.fn() : () => {},
     getState: () => ({
         routes: [],
     }),
@@ -30,17 +32,17 @@ type NativeNavigationMock = typeof ReactNavigation & {
 };

 export * from '@react-navigation/core';
-const Link = realReactNavigation.Link;
-const LinkingContext = realReactNavigation.LinkingContext;
-const NavigationContainer = realReactNavigation.NavigationContainer;
-const ServerContainer = realReactNavigation.ServerContainer;
-const DarkTheme = realReactNavigation.DarkTheme;
-const DefaultTheme = realReactNavigation.DefaultTheme;
-const ThemeProvider = realReactNavigation.ThemeProvider;
-const useLinkBuilder = realReactNavigation.useLinkBuilder;
-const useLinkProps = realReactNavigation.useLinkProps;
-const useLinkTo = realReactNavigation.useLinkTo;
-const useScrollToTop = realReactNavigation.useScrollToTop;
+const Link = isJestEnv ? realReactNavigation.Link : () => null;
+const LinkingContext = isJestEnv ? realReactNavigation.LinkingContext : () => null;
+const NavigationContainer = isJestEnv ? realReactNavigation.NavigationContainer : () => null;
+const ServerContainer = isJestEnv ? realReactNavigation.ServerContainer : () => null;
+const DarkTheme = isJestEnv ? realReactNavigation.DarkTheme : {};
+const DefaultTheme = isJestEnv ? realReactNavigation.DefaultTheme : {};
+const ThemeProvider = isJestEnv ? realReactNavigation.ThemeProvider : () => null;
+const useLinkBuilder = isJestEnv ? realReactNavigation.useLinkBuilder : () => () => '';
+const useLinkProps = isJestEnv ? realReactNavigation.useLinkProps : () => ({});
Anaslancer commented 6 days ago

@ishpaul777 I think it's best solution and it's similar like my above proposal.

ishpaul777 commented 5 days ago

ok lets go with this solution https://github.com/Expensify/App/issues/51108#issuecomment-2432375693, lets assign @Anaslancer for PR

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 5 days ago

Triggered auto assignment to @yuwenmemon, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

trjExpensify commented 5 days ago

Not really sure where to put this, but I guess being dev related we can put it in #quality.

chiragsalian commented 5 days ago

LGTM, feel free to create the PR @Anaslancer.

Anaslancer commented 4 days ago

@chiragsalian I can't not create a pull request from my forked repo's branch to the original repo's staging branch. Is it okay to create to the original repo's main branch?

chiragsalian commented 4 days ago

Not sure i follow. Like yeah you should not create a PR from your repo to be merged to our repo's staging branch. And yes, creating a PR from your repo to be merged to our repo's main branch is the way to go 🙂 If it helps - all PRs are merged to our main branch. Internal deployers move all the new main code to staging once a day so that QA can test it. Then once its QA's our internal deployers will move all the new staging code to production once a day.

Anaslancer commented 3 days ago

@chiragsalian I've just created a pull request. BTW, I have a question. This ticket has not a upwork job link. I don't have a chance to hire on upwork for this ticket?

ishpaul777 commented 3 days ago

@Anaslancer it looks like your PR does not include any changes related to solution it includes unnecessary changes, can you please check again ??


This ticket has not a upwork job link. I don't have a chance to hire on upwork for this ticket?

Looks like automation failed here but dont worry BZ team member (@trjExpensify) will take care of this, after 7-days of PR is deployed, you will be hired paid for the job.

trjExpensify commented 18 hours ago

Yep, PR has merged. Once it goes to prod, the 7-day regression period will start. Then payment will be made 👍