-
-
Notifications
You must be signed in to change notification settings - Fork 90
Version 2.0 #212
Version 2.0 #212
Conversation
* Test cleaning up reactions for uncommitted components * First attempt at a fix for cleaning up reactions from uncommitted components * Use debounce instead of fixed interval * Add unit test for missing observable changes before useEffect runs This is a test to check that observable changes made between the first component render and commit are not lost. It currently fails (and did so before the change in PR #119) * Add test for cleanup timer firing too early for some components This demonstrates (in a slightly contrived way) how, if the cleanup timer fires between a recent component being rendered and it being committed, that it would incorrectly tidy up a reaction for a soon-to-be-committed component. * Update test for missing changes to check Strict and non-Strict mode We had an existing test to check that observable changes between render and commit didn't go missing, but it only checked in Strict mode, and there's a problem with non-Strict mode. * Add cleanup tracking and more tests This adds full cleanup tracking, and even more tests: - we now track how long ago potentially leaked reactions were created, and only clean those that were leaked 'a while ago' - if a reaction is incorrectly disposed because a component went away for a very long time and came back again later (in a way React doesn't even do right now), we safely recreate it and re-render - trap the situation where a change is made to a tracked observable between first render and commit (where we couldn't force an update because we hadn't _been_ committed) and force a re-render - more unit tests * Fix renamed test file When I renamed this file, I forgot the .test. suffix. D'oh. * Extract tracking and cleanup logic out to separate file * Update src/useObserver.ts Co-Authored-By: RoystonS <royston@shufflebotham.org> * Move some more tracking internals into the tracking code
LGTM, just I think that there could be an optimization in the cleanup code. |
@xaviergonz I am sure it can be improved later. Such internal optimization won't be a breaking change most likely. But feel free to submit a PR if you like. |
Btw, since it is now using tsdx I have the hunch .browserlistrc, tslint.config and jest config files are no longer needed |
And probably it might be worth to add a .eslintrc.js with
if not in the package json already |
TSDX is just a build tool, the Jest config is still needed for sure, the |
Just checking in, was this waiting for something from my side? (Github notifications stopped working for me years ago) |
@mweststrate No, I think this is mostly on me. I got kinda busy and forgot about this. Want to finish up #214 first and now also #226 should be figured out first. |
Alright, almost ready for release, just waiting for confirmation if the fix for #226 works. |
Given that the next branch solved some problems for others, I guess we can also cut a release and deal with any fallout later? I doubt we get more feedback without releasing. |
@mweststrate Well, given the Concurrent is now available for the library authors, I think it would be responsible to test it out properly with it before going full 2.0. However, right now the |
Thanks a lot for the feedback, not sure how could that happen. Fixed in 2.0.0-alpha.4 |
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.
can you rebase it?
@auvipy Not before figuring out if there are some issues with the Concurrent mode. |
I think it's time to get this thing out. It seems that React 16.9 has included some preliminary changes for upcoming Concurrent mode which are causing trouble with the current test.
With the code of
useObserver
fromnext
branch, tests are passing again although there is a bunch ofact
warnings which might be false positive as it's actually necessary to simulate commit phases and similar. We can review that later.Personally, we have been using this branch for a couple of months now in production and no related problem seems to occur.
Changelog follows:
This branch has diverted from
master
fair amount and is really hard to rebase. For any further updates just merge master directly and we will squash later.