-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Chore: Upgrade to react 18 #64428
Chore: Upgrade to react 18 #64428
Changes from 39 commits
0a6064b
bd325e0
ae4f4d7
a679c74
7a8e1a9
7652c93
0549d8b
8b8232e
570d594
cc01302
747b50e
cb0ca7e
423483a
13f45f5
846b883
7b97fab
53f29f0
2b9d8dd
e3c14d1
ceb9d7d
d6bac45
baff119
9ab3067
71ef071
1b9f4c1
7c39cda
0c39815
9606b06
31b59c4
7afe79b
0c45018
46f99d1
2438f13
3f16481
307f3c9
ffa4ecb
b2d103b
34cdf50
cabc9b6
3324cea
09633dc
36f2511
ac09842
f2580ef
8d4f518
cee4037
bba9118
2aa2ddd
711598a
9c5fc2d
90dd670
c94bc79
b03b23a
b08bb9c
1b71bfb
56a874d
346151d
8a8b919
2fcb339
cfc57b0
b20f267
d1483a4
8f3a402
f633691
a565d87
3fe7f61
7655c7e
3fe8563
6d1433d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
diff --git a/dist/ts3.9/blocks/DocsContainer.d.ts b/dist/ts3.9/blocks/DocsContainer.d.ts | ||
index be330e44bebb02eaf2c92d365d4e7dc1da452465..6c8b1d42bea2e184456e2757eb2ee20076ba43b3 100644 | ||
--- a/dist/ts3.9/blocks/DocsContainer.d.ts | ||
+++ b/dist/ts3.9/blocks/DocsContainer.d.ts | ||
@@ -1,7 +1,8 @@ | ||
-import { FunctionComponent } from 'react'; | ||
+import { FunctionComponent, ReactNode } from 'react'; | ||
import { AnyFramework } from '@storybook/csf'; | ||
import { DocsContextProps } from './DocsContext'; | ||
export interface DocsContainerProps<TFramework extends AnyFramework = AnyFramework> { | ||
context: DocsContextProps<TFramework>; | ||
+ children?: ReactNode; | ||
} | ||
export declare const DocsContainer: FunctionComponent<DocsContainerProps>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
diff --git a/index.d.ts b/index.d.ts | ||
index d116f54d6da12d24b48e24ff3636c9066059aa58..93290945d8b1818cab893d6466179b33869a47b9 100644 | ||
--- a/index.d.ts | ||
+++ b/index.d.ts | ||
@@ -25,6 +25,7 @@ export type SplitPaneProps = { | ||
pane2Style?: React.CSSProperties; | ||
resizerClassName?: string; | ||
step?: number; | ||
+ children?: React.ReactNode; | ||
}; | ||
|
||
export type SplitPaneState = { |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ describe('ErrorBoundary', () => { | |
expect(context.contexts.react.componentStack).toMatch(/^\s+at ErrorThrower (.*)\s+at ErrorBoundary (.*)\s*$/); | ||
}); | ||
|
||
it('should recover when when recover props change', async () => { | ||
it('should rerender when recover props change', async () => { | ||
const problem = new Error('things went terribly wrong'); | ||
let renderCount = 0; | ||
|
||
|
@@ -64,6 +64,8 @@ describe('ErrorBoundary', () => { | |
); | ||
|
||
await screen.findByText(problem.message); | ||
expect(renderCount).toBeGreaterThan(0); | ||
const oldRenderCount = renderCount; | ||
|
||
rerender( | ||
<ErrorBoundary dependencies={[1, 3]}> | ||
|
@@ -78,6 +80,6 @@ describe('ErrorBoundary', () => { | |
</ErrorBoundary> | ||
); | ||
|
||
expect(renderCount).toBe(2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems to be rendering twice as much in react 18. not sure whether this is exposing a problem or not, but explicitly counting the renders feels a little brittle to me 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've also seen this with plugins where a PR updates |
||
expect(renderCount).toBeGreaterThan(oldRenderCount); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ const expanderContainerStyles = css` | |
height: 100%; | ||
`; | ||
|
||
export function ExpanderCell<K extends object>({ row, __rowID }: CellProps<K, void> & { __rowID: string }) { | ||
export function ExpanderCell<K extends object>({ row, __rowID }: CellProps<K, void>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
return ( | ||
<div className={expanderContainerStyles}> | ||
<IconButton | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RTL is built on top
testing-library/dom
, so should include all its functions. I quickly checked our codebase and it seems like we usetesting-library/dom
mostly for thewithin
function, which is also available fromtesting-library/react
. Should we try to remove this package?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so i've changed the instances of the code that are using
within
/fireEvent
from@testing-library/dom
directly to instead use the methods from@testing-library/react
, however i don't think we can remove the package itself as@testing-library/user-event
declares a peer dependency on it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
@testing-library/react
should pull it in as a dependency? Also found an issue where it says that@testing-library/dom
shouldn't be listed as dependency unless used directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does, but that will still leave us with warnings when yarn installing:
there's another issue here saying that installing it directly is the correct approach when using
@testing-library/user-event
.