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
Use Lodash's mergeWith and warn when losing reactivity #591
Conversation
Codecov Report
@@ Coverage Diff @@
## master #591 +/- ##
==========================================
- Coverage 81.66% 81.61% -0.05%
==========================================
Files 66 66
Lines 1320 1322 +2
Branches 81 81
==========================================
+ Hits 1078 1079 +1
- Misses 223 224 +1
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
With this change, and without #586 , when we hold or release a workflow we get |
I think this should be ready for review. Just waiting for CI to complete, and codecov to confirm the code is covered. @hjoliver after this change, we should see a warning, until #586 is merged. After #586 is merged, we shouldn't — in theory — see any warning again. But if we see the warning in the console, we won't have to spend time investigating reactivity issues in deltas (no fun!) 👍 |
Perhaps we have a flaky test, which test was failing? |
The Today: https://github.com/cylc/cylc-ui/actions/runs/555846220
From 2 days ago, this branch too: https://github.com/cylc/cylc-ui/actions/runs/552663688
|
Ok, have an idea what might be causing that, #592 |
Tried upgrading to Yarn2, however, got a load of errors, will need to go through the migration guide properly. Perhaps worth waiting for Yarn2 to stabilise first, dumping in an issue for now. |
Test failure nothing to do with this branch. |
I think I found an alternative to this PR, that would add reactivity to an object, instead of just warning. Testing will take a little bit (browser + IDE) |
I'm pushing an improved solution to #530 as that branches also has issues with reactivity after deltas are merged. Once that one is merged, will leave this one open a little longer while I look at other PR's, to make sure this issue is really gone. |
So far looks like the new solution worked on #530 . If that PR is closed, then this one can be closed as superseded (it has the same code, with a small modification in the merge customizer using |
Confirmed it's closed by #530 |
These changes close #589 (and removes an unnecessary
console.log
from tests)Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.