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.

Sign up for GitHub

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

Jump to bottom

[WIP] Support React 18 #3005

Closed
wants to merge 1 commit into from
Closed

[WIP] Support React 18 #3005

wants to merge 1 commit into from

Conversation

mweststrate
Copy link
Member

@mweststrate mweststrate commented Jun 24, 2021 •
edited
Loading

First attempt at full React 18 support.

Didn't get very far since react-testing-library isn't updated yet :)

@changeset-bot
Copy link

changeset-bot bot commented Jun 24, 2021

⚠️ No Changeset found

Latest commit: 7b068fe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mweststrate mweststrate mentioned this pull request Jun 24, 2021
Closed
@markerikson markerikson mentioned this pull request Jun 28, 2021
Closed
@mweststrate
Copy link
Member Author

Highlight impact of startTransaction updates vs local state: https://codesandbox.io/s/usedeferredvalue-issue-iteration-forked-ovvsl / https://codesandbox.io/s/usedeferredvalue-issue-iteration-forked-li6uj?file=/src/index.js

@zhangchao828
Copy link

tearing scenarios

Is this problem difficult to solve in mobx ?

@urugator
Copy link
Collaborator

Is this problem difficult to solve in mobx ?

My current understanding (and could be wrong) is that if you want to avoid tearing, the component can not depend on mutable state. I don't think that's reasonably doable with mobx.

@janryWang janryWang mentioned this pull request Jul 19, 2021
Closed
1 task
@airhorns
Copy link

testing-library/react-testing-library#509 has been closed with their new alpha now 🥳

@zhangchao828
Copy link

React 18 is now in beta
is there any progress

@franckchen
Copy link

React 18 is now in beta is there any progress

The author has actually made it clearer .mobx works fine with React 18. And I don’t think we need to be impatient, I dont think online project will use React concurrent mode quickly, i think there will be a lot of works to do. even if project upgrade React to ^18.0.0. i think we still use Legacy Mode or Block Mode.

and i made some test. yes, there are some tearing problem in cooperating with mobx and react 18 concurrent mode. but the final render result is ok.

@urugator
Copy link
Collaborator

urugator commented Jan 2, 2022

replace unstable_batched_updates with flushSync

  1. Why is flushSync necessary?
  2. flusSync warns when called from lifecycle methods, so how do we run action from eg useEffect without warning?

@samcooke98
Copy link

re; > test against https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering

I implemented it here; https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering/pull/63/files against the current released version of mobX.

@macbem
Copy link

macbem commented Apr 4, 2022

@mweststrate what's the status of this? Can we expect a release soon? If there are any "good first issues" to tackle that would help prep this, can you share a list?

@mweststrate
Copy link
Member Author

Sorry haven't really been following along React 18 anymore in the past year, so lost most of the context I had on it😅. The primarily thing that needs to happen is to bump the peer deps and unit tests to use React 18, so PR for that is welcome!

@noah79
Copy link

noah79 commented Apr 5, 2022

Is this really true? We had to switch back to ReactDOM.render() today after hitting cases where mobx observer components wouldn't rerender randomly even though their underlying observables were changing. Mind you we're on Mobx 5.15.7 + mobx-react-lite 2.2.2. However this thread doesn't lead me to believe that going through the gargantuan task of updating to latest and greatest mobx will somehow resolve the issue: #2526

Would appreciate guidance here as we are hugely dependent on mobx (250 occurrences of useLocalStore in the codebase) and switching to useState as is suggested in the above thread sounds worse than a root canal (or 4).

@mweststrate
Copy link
Member Author

mweststrate commented Apr 5, 2022 via email

@airhorns
Copy link

airhorns commented Apr 5, 2022

@mweststrate do you or you know any community contributors who do freelancing / consulting? My company would love to do a contract with them to flesh out support! I think there's probably some challenges around all the useSyncExternalStore that need to be worked out to avoid tearing problems, I've seen some murky stuff inside valtio and use-query and some other libraries that do some intense state management outside of react that makes me think its pretty involved, so I'd love to work with someone who knows mobx really well and can learn the ins and outs of that stuff.

@noah79
Copy link

noah79 commented Apr 5, 2022 •
edited
Loading

@airhorns We're in the same boat. We could pool resources if someone qualified is available to truly take this on.

@mweststrate The breakages are psuedorandom. Usually related to a query returning during a render in our complex MDI which then changes a prop wrapped by useAsObservableSource in a parent store which then the child component isn't realizing it has changed and thus is rendering stale data. Not sure how I would go about producing a minimum example as things 98% worked but then mysteriously would break after a seemingly unrelated change elsewhere that I believe subtly changed timings or render paths.

@kubk
Copy link
Collaborator

kubk commented Apr 5, 2022 •
edited
Loading

@noah79 useAsObservableSource is deprecated: https://github.com/mobxjs/mobx/blob/main/packages/mobx-react-lite/src/useAsObservableSource.ts#L8-L9

I follow a simple rule - do not mix mutable Mobx with immutable React state / props. I use Mobx for everything, even for local state, this is how I avoid useAsObservableSource. But unless we have a codesandbox with a reproducible error - there isn't much we can help. It'd be nice if you can set it up and create an issue out of it.

UPD: For anyone interested, I've created a demo with Mobx + React 18 and so far it just works with no issues: https://codesandbox.io/s/vigilant-hellman-3mjsty?file=/index.js

@noah79
Copy link

noah79 commented Apr 5, 2022

@kubk I'm aware it's deprecated. We're still happily on Mobx 5.15.7 and have seen no reason to upgrade and lose decorators, be forced to use makeAutoObservable, etc. I considered upgrading this week due to React 18 breakages, but nothing I've read here leads me to believe it's going to actually work for all of our complex cases. Your codepen is super simplistic compared to our running app (A customizable MDI that sells for 7 figures and is used by companies like Swarovski, ASICS, Restoration Hardware, Under Armor and others). If I knew that it was bulletproof against concurrency mode then I might take the plunge and rewrite the literally hundreds of stores that use mobx, but until I see true concurrency support, it's a ton of work for likely zero tangible benefit. Hell the benchmarks say Mobx 6 is slower than what we're on! Decorators are even moving forward in javascript so I have a feeling someday a mobx 7 will come out that will embrace decorators again and the API will (hopefully!) return to the much kinder one that preddated version 6.

I like my decorators. I dislike needing to call magic methods in my constructors, parent classes, etc. I like knowing if a class instance variable is actually observable or not (decorated or not), etc.

It ain't deprecated in v5.15.7 :)

image

@mweststrate
Copy link
Member Author

lose decorators

Decorators are supported in 6 as well

it's a ton of work

Tried the codemod?

It ain't deprecated in v5.15.7 :)

You know that that isn't the meaning of the word deprecated ;)

useAsObservableSource has some flaws in it that are even more problematic in concurrent mode, so you'd have to get rid of those one way or another in any case I'm afraid.

@kubk
Copy link
Collaborator

kubk commented Apr 13, 2022 •
edited
Loading

@mweststrate Hi, there is a new Create-React-App release that ships with React 18, so it's probably the time to allow installing Mobx alongside React 18. I'd like to simply bump allowed react versions in mobx-react-lite and mobx-react. Should I create a separate PR or continue working here?

@mweststrate
Copy link
Member Author

mweststrate commented Apr 13, 2022 via email

@akphi akphi mentioned this pull request Apr 21, 2022
Closed
13 tasks
@mweststrate mweststrate mentioned this pull request May 3, 2022
Merged
@mweststrate
Copy link
Member Author

Being superseded by #3387

@github-actions github-actions bot mentioned this pull request May 6, 2022
Merged
@mweststrate
Copy link
Member Author

Should be fixed in mobx-react-lite@3.4.0 / mobx-react@7.4.0

@mweststrate mweststrate closed this May 6, 2022
@Ricbet Ricbet mentioned this pull request Mar 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kubk kubk kubk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mweststrate @zhangchao828 @urugator @airhorns @franckchen @samcooke98 @macbem @noah79 @kubk