-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
React 19 support #3985
React 19 support #3985
Conversation
🦋 Changeset detectedLatest commit: f1dbfeb The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
c4e02b1
to
b9da1d6
Compare
93a3ecd
to
92b9b2d
Compare
@@ -377,7 +377,7 @@ describe("is used to keep observable within component body", () => { | |||
}) | |||
expect(container.querySelector("span")!.innerHTML).toBe("22") | |||
expect(counterRender).toBe(2) | |||
expect(observerRender).toBe(3) |
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.
React now renders the observer an extra time for some reason. The reaction only fires once though when the store mutates
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.
these hooks are all basically anti-pattern anyway, so good enough :)
if (children && render) { | ||
console.error( | ||
"MobX Observer: Do not use children and render in the same time in `Observer`" | ||
) | ||
} |
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.
propTypes
removed in v19 so I've duped this inside the render body
@@ -49,7 +49,7 @@ test("computed properties react to props when using hooks", async () => { | |||
act(() => { | |||
jest.runAllTimers() | |||
}) | |||
expect(seen).toEqual(["parent", 0, "parent", 2]) | |||
expect(seen).toEqual(["parent", 0, "parent", 2, 2]) |
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.
}, [multiplier]) | ||
useEffect(() => setMultiplier(3), []) | ||
}, | ||
{ |
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.
NIT: This pattern reoccures quite a few times in the test, might be nice to make a utility for it? (separate PR is fine if interested)
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.
This looks great, thanks so much for picking this up!
interface IObserverProps { | ||
children?(): React.ReactElement | null | ||
render?(): React.ReactElement | null | ||
} | ||
|
||
function ObserverComponent({ children, render }: IObserverProps) { | ||
if (children && render) { | ||
console.error( | ||
"MobX Observer: Do not use children and render in the same time in `Observer`" |
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.
typo: in the same time
-> at the same time
Code change checklist
UpdatedN/A/docs
. For new functionality, at leastAPI.md
should be updatedyarn mobx test:performance
)Note: before merging, I think I should also dup any
propType
checks inside render to be compatible across multiple React versions. I also intentionally haven't bumped the types forreact
andreact-dom
to keep the propTypes types such asValidator
,Requireable
etc.