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

Upgrade React version of manager #20420

Closed

Conversation

shogohida
Copy link

@shogohida shogohida commented Dec 27, 2022

Issue: #20378

Signed-off-by: Shogo Hida shogo.hida@gmail.com

What I did

Upgraded React version of manager

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
@shilman shilman added ui maintenance User-facing maintenance tasks labels Dec 27, 2022
@thebuilder
Copy link
Contributor

This will require a couple of other changes:
https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html

Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
@ndelangen ndelangen self-requested a review January 13, 2023 18:59
@ndelangen ndelangen self-assigned this Jan 13, 2023
@ndelangen
Copy link
Member

@shogohida I'd expect changes here:

export function renderStorybookUI(domNode: HTMLElement, provider: Provider) {
if (!(provider instanceof Provider)) {
throw new Error('provider is not extended from the base Provider');
}
ReactDOM.render(<Root key="root" provider={provider} />, domNode);
}

Or is my assumption incorrect?

@ndelangen
Copy link
Member

@shogohida will you be making the required changes @thebuilder mentioned on this PR?

I'm very excited to get storybook's manager UI upgraded to react18! But this will need more work to get merged, and we definitely do not want this be much closer to the release-date; in fact I suspect @shilman might already find it too risky (even though it's the manager, addons could potentially break..?)

Can you tell me your plans for this PR? If you're stuck on anything please reach out to me on Discord. 🙏

@shogohida
Copy link
Author

shogohida commented Jan 13, 2023

@ndelangen
Thanks for your comment and sorry for keeping you guys waiting for a long time. I will try to work today and tomorrow and if I get stuck, I will reach you on Discord. I'm new to Storybook so it's probable. How should I contact you on Discord?

I was busy with my job and left this issue undone 🙇 Sorry about that.

Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
@ndelangen
Copy link
Member

Please don't feel sorry, your efforts are appreciated!

My name is the same on discord, I'm usually online. just ping me.. You can also schedule a meeting with me here:
https://calendly.com/chromaui

@shogohida
Copy link
Author

@ndelangen
I made changes. Is my understanding correct?

9dcf29a

Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
node_modules/.package-lock.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
code/ui/manager/src/index.tsx Show resolved Hide resolved
@ndelangen
Copy link
Member

Related #20402 #17215 #20466

Signed-off-by: Shogo Hida <shogo.hida@gmail.com>
@shogohida shogohida marked this pull request as ready for review January 14, 2023 11:24
@ndelangen ndelangen self-requested a review January 14, 2023 12:38
@ndelangen ndelangen dismissed their stale review January 14, 2023 12:38

npm lockfiles were removed

@shogohida
Copy link
Author

@ndelangen
Should I recover npm lockfiles? I ask this because you dismissed your review.

In addition, I checked failing tests but I don't know how to solve them....

@ndelangen
Copy link
Member

Nope the npm lock files were removed, so I deleted the negative review. Perhaps we can ask for help from other contributors?

@shogohida
Copy link
Author

shogohida commented Jan 14, 2023

@shogohida
Copy link
Author

shogohida commented Jan 16, 2023

I read error messages of failing tests and looked for solutions on Google. I write down notes here from my investigations so that other contributors can easily see what happened.

Notes:

  • Failing tests of e2e-sandboxes
    • The error message is locator.waitFor: Target closed
    • It seems they are related to a bug or something of Playwright so I'm not sure if the failing tests of e2e-sandboxes were caused by the changes of this PR.
  • Failing tests of cra-bench and react-vite-bench
    • The error message is Too long with no output (exceeded 10m0s): context deadline exceeded
    • This is related to CircleCI settings according to this article
    • I'm not sure if this PR caused test failures.
  • Failing tests of storybook-ui
    • The error message is Error: Evaluation failed: Object at ExecutionContext._evaluateInternal (/var/task/capture-v4/build/node_modules/puppeteer-core/lib/cjs/puppeteer/common/ExecutionContext.js:218:19
    • They are related to Puppeteer
    • I don't know how to solve these too

@ndelangen
Copy link
Member

If you start storybook on this branch, it simply doesn't render anything at all:
https://635781f3500dd2c49e189caf-ltllzjqyji.chromatic.com/

Comment on lines +55 to +57
"dependencies": {
"@types/react-dom": "^18.0.10"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, I think this can be moved into devDependencies

@virtuoushub
Copy link
Contributor

Not 💯% on this as I have not tested it out myself, but after reading this comment, I think this PR might benefit from a similar approach to #21197

@valentinpalkovic
Copy link
Contributor

Closing in favour of #21673

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants