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

Revert "Revert "storybook v6 -> v7"" #1258

Merged
merged 4 commits into from
Apr 26, 2023

Conversation

takurinton
Copy link
Contributor

@takurinton takurinton commented Apr 25, 2023

Reverts #1257

CustomCell の部分について調査する

@netlify
Copy link

netlify bot commented Apr 25, 2023

Deploy Preview for ingred-ui ready!

Name Link
🔨 Latest commit 90c7e4c
🔍 Latest deploy log https://app.netlify.com/sites/ingred-ui/deploys/64487774f0d4a0000868adbb
😎 Deploy Preview https://deploy-preview-1258--ingred-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@takurinton
Copy link
Contributor Author

takurinton commented Apr 25, 2023

websocket が繋がってないのは同じだけど、Event に少し差がある(とはいえ時間くらいであとは一緒、type error になってても表示される)
うまく表示される画面
image

表示されない画面(CustomCell)
image

@takurinton
Copy link
Contributor Author

開いたまましばらく放置(1分ほど)するとデバッガで以下の文言
image

@takurinton
Copy link
Contributor Author

どこから同様の状態になるかをバージョンを下げながら確認したけど、v7.0.0 の時点では出てるのでそれが起因であることは間違いなさそう。

@takurinton
Copy link
Contributor Author

これひょっとしたら再現性がないかもしれない
#1258 (comment)

ローカルの Chrome(Mac M1 v12.2)だと3回に1回くらいクラッシュしてしまう。
image

@takurinton
Copy link
Contributor Author

メモリクラッシュ前に一時停止しましたが出ると UI 自体は表示される...??
image

これだとちょっと判断が難しそう、相変わらず console は静か(build のログはでる)

@takurinton
Copy link
Contributor Author

愚直にぽちぽちしたけど、おそらくクラッシュするのは CustomCell を指定した時の DataTable だけ

@takurinton
Copy link
Contributor Author

render: (_args) => {
render: () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused の引数を削ったら動いたんだけど、他のコンポーネントではそうではないのでいずれも再現性はなさそう
謎が深まった

package.json Outdated
Comment on lines 49 to 54
"@storybook/addon-essentials": "7.1.0-alpha.9",
"@storybook/addon-links": "7.1.0-alpha.9",
"@storybook/addon-mdx-gfm": "7.1.0-alpha.9",
"@storybook/addon-storysource": "7.1.0-alpha.9",
"@storybook/react": "7.1.0-alpha.9",
"@storybook/react-webpack5": "7.1.0-alpha.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

さっき新しいバージョン出てたのでついでにあげた。
7.1.0 がリリースされたら alpha 消す作業する(dependabot が通知してくれそうだけど)

injectStoryParameters: false,
},
},
},
"@storybook/addon-mdx-gfm",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@penicillin0
前回の自分のレビュー漏れで申し訳ないんですけど、これってなんで入ってるんでしたっけ?

Copy link
Contributor

Choose a reason for hiding this comment

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

auto migrationに従ってGithub Flavored Markdownを使う場合はいれておけよと言われていれました

markdownのところは非推奨になっていて変えるはずなので、将来的にはいらなくなるかも

@takurinton
Copy link
Contributor Author

一応前コンポーネント問題なく表示はされる
DataTable の CustomCell みたいなクラッシュする系の壊れ方はしない

Comment on lines -11 to -20
"@storybook/addon-postcss",
{
// MEMO: included in addon-essentials
name: "@storybook/addon-docs",
options: {
sourceLoaderOptions: {
injectStoryParameters: false,
},
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

addon-essentials に内包されてるからなくて良さそう

Copy link
Contributor

@youchann youchann left a comment

Choose a reason for hiding this comment

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

thanks!!!!!!!!!

@takurinton takurinton merged commit e8a14a4 into master Apr 26, 2023
@takurinton takurinton deleted the revert-1257-revert-1214-storybook-v7 branch April 26, 2023 01:56
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.

3 participants