-
-
Notifications
You must be signed in to change notification settings - Fork 632
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
[Not For Merge] alternate impl for #2827 #2867
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
Size Change: -66 B (-0.07%) Total Size: 91.1 kB
ℹ️ View Unchanged
|
Preview in LiveCodesLatest commit: 2c26d84
See documentations for usage instructions. |
All tests are passing. We need to create a test to break it. |
I don't think it does. There's a couple issues that I see with this implementation:
I don't know if we have test cases for these. Maybe we should add them. After addressing these two issues, I think this implementation will be similar to #2827. |
2 was my concern too. it requires a priority queue instead of a set. though, with the react use case, it's not a problem because useReducer dispatch is async. a new test with store api should fail. 1 should be fine unless we want to make different behaviors between mounted atoms and unmounted atoms. |
Let me continue to iterate on #2827. I'll have some failing tests later today that we can use as a North star.
I was surprised to see no failing test, tbh. There is definitely a gap in our testing here. The |
That'd be good, and I will cherry-pick commits into this branch.
We are rowing our boats in the ocean from different directions. |
Update: I am unable to break this implementation. Nice work. |
I will try to break 2 and then fix. |
moved to dmaskasky#1 |
/ecosystem-ci run |
Ecosystem CI Output
|
does this work?
ref: #2827