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

[HOLD for payment 2022-01-25] Storybook - Errors appear in the JS console when tap any default page Canvas/ Docs #7187

Closed
kbecciv opened this issue Jan 13, 2022 · 18 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jan 13, 2022

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


Action Performed:

  1. Go to https://staging.new.expensify.com/docs/index.html
    and verify that you can see the storybook docs with the themes matching the screenshots below.
  2. Canvas / Header/ Default
  3. Canvas / ButtonWithDropdwn/ Fefault
  4. Canvas/ MenuItem/ Default
  5. Repeat step 2-4 for Docs tab
  6. Go to HeaderWithCloseButton

Expected Result:

Verify that no errors appear in the JS console and HeaderWithCloseButton component renders without a problem.

Actual Result:

Errors appear in the JS console and the HeaderWithCloseButton component does not render due to an error.

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.1. 29.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation



image

Expensify/Expensify Issue URL: Applause

Issue reported by:

Slack conversation:

Issue was found when executing: #7154

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @mountiny (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels Jan 13, 2022
@mountiny
Copy link
Contributor

mountiny commented Jan 14, 2022

When testing locally as well as on staging, there are the following errors showing up in the console:
image

image

I could not replicate the same errors which are shown on staging in dev (running, not built storybook).

I have updated the issue description to also include the broken scenario for HeaderWithCloseButton component, where the translate props are not passed.

It might be trickier to identify the other error, but it would be nice to get someone External to look into this, it is a low priority task.

cc @roryabraham since you have worked on the recent rehaul of the Storybook.

@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Jan 14, 2022
@MelvinBot
Copy link

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@parasharrajat
Copy link
Member

Proposal

  1. HeaderWithCloseButton component usages withLocalize HOC which depends on Onyx data vai withOnyx to work properly.
  2. We changed our app logic to use the Context-Provider pattern to speed up the app and WithLocalize also went through this evolution.
  3. Thus as a result it requires LocaleContextProvider in the parent Component Tree to work.
  4. In our stores, we do not wrap HeaderWithCloseButton story in this provider and thus it fails.

The Fix

  1. Now, we can create a local decorator for this story only but I believe that other components may also need this.
  2. Thus I will create a global decorator in .storybook/preview.js file.
const decorators = [
    Story => (
        <ComposeProviders
            components={[
                OnyxProvider,
                LocaleContextProvider,
            ]}
        >
            <Story />
        </ComposeProviders>
    ),
];
  1. Now, LocaleContextProvider internally usages withOnyx and WithOnyx hocs will prevent the render on any child component until the Onyx's internal state is ready. So we need to also initialize the Onyx.
  2. We can easily pollute the global scope as we don't want any shutdown mechanism for stories.
    we can simply do that in preview.js via
Onyx.init({
    keys: ONYXKEYS,
});
  1. And adding the OnyxProvider in the decorator shown in the above snippets.

This will fix the errors and as a plus, we can create stories for any component that depends on OnyxData. Just add the mock data in Onyx.Init statement and voila.

@mountiny
Copy link
Contributor

@parasharrajat This looks great, but just to clarify. Will this fix the errors shown in the console, which do not have necessarily any negative effect on the app afaik, but we would still like to get rid of them.

@parasharrajat
Copy link
Member

Yeah. It will resolve all of them for storyBook. But it does not affect the Warnings that are coming in our app. Those warnings will be removed when we will update Urban Airship lib.

@mountiny
Copy link
Contributor

@parasharrajat Great! Feel free to start working on the PR. Nicole will create the jobs later on.

@NicMendonca
Copy link
Contributor

Sorry for the delay everyone! Here is the job: https://www.upwork.com/jobs/~0132a07ec55f3a9411

@botify botify removed the Daily KSv2 label Jan 14, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Jan 14, 2022
@MelvinBot
Copy link

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 14, 2022
@MelvinBot
Copy link

Current assignee @mountiny is eligible for the Exported assigner, not assigning anyone new.

@mountiny
Copy link
Contributor

No worries @NicMendonca, this one was a speedy one. @parasharrajat Can you please apply for the job?

@parasharrajat
Copy link
Member

Done.

@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 14, 2022
@mountiny
Copy link
Contributor

@NicMendonca Can you please hire Rajat for this job? Thank you very much!

@NicMendonca
Copy link
Contributor

hired!

@mountiny mountiny added the Reviewing Has a PR in review label Jan 18, 2022
@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 18, 2022
@botify botify changed the title Storybook - Errors appear in the JS console when tap any default page Canvas/ Docs [HOLD for payment 2022-01-25] Storybook - Errors appear in the JS console when tap any default page Canvas/ Docs Jan 18, 2022
@botify
Copy link

botify commented Jan 18, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.30-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-01-25. 🎊

@NicMendonca
Copy link
Contributor

@parasharrajat paid! thank you!

@kbecciv kbecciv reopened this Mar 24, 2022
@kbecciv
Copy link
Author

kbecciv commented Mar 24, 2022

@mountiny Issue reproduced during Regression with build 1.1.46.0

Recording.113.mp4

@mountiny
Copy link
Contributor

@kbecciv Can you please comment on some PRs from the release which worked with Storybook, there is a couple I am not sure which this is related to, but this is an old issue and if this is a regression that now appeared in staging, it must be related to the deployed PRs and not here. I will close this issue, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants