-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore: improve handling of rollback scenarios #9680
Conversation
Performance Report for 8dd4ec3 Scenario - basic-record-materialization: ☑️ Performance is stable
Scenario - relationship-materialization-simple: ☑️ Performance is stable
Scenario - relationship-materialization-complex: ☑️ Performance is stable
Scenario - unload: ☑️ Performance is stable
Scenario - unload-all: ☑️ Performance is stable
Scenario - destroy: ✅ Performance improved
Scenario - add-children: ✅ Performance improved
Scenario - unused-relationships: ✅ Performance improved
|
Commit v Release Performance Report for 8dd4ec3 Scenario - basic-record-materialization: ✅ Performance improved
Scenario - relationship-materialization-simple: ☑️ Performance is stable
Scenario - relationship-materialization-complex: ☑️ Performance is stable
Scenario - unload: ✅ Performance improved
Scenario - unload-all: ✅ Performance improved
Scenario - destroy: ✅ Performance improved
Scenario - add-children: ✅ Performance improved
Scenario - unused-relationships: ✅ Performance improved
|
} | ||
} else { | ||
changed = true; | ||
} |
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.
@runspired Why do we need this if else when you are performing same operations inside ?
Can we just do it with just one if condition like below
if (DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE) {
if ((!remoteClearsLocal && i < priorLocalLength) || i < priorLocalLength || remoteClearsLocal ) {
if (!changed && j < priorLocalLength) {
const priorLocalMember = priorLocalState![j];
changed = priorLocalMember !== member ? true : false;
}
}
}
}
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.
!remoteClearsLocal &&
is the difference. The way we approach deprecation flagging (so that deprecated code can be fully eliminated) is to wrap the full version of the deprecated code in its flag and the else be the full new version.
RE changed = priorLocalMember !== member ? true : false;
, we don't do this because once changed
is true we never revert it to false.
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.
@runspired ok , in that case make it as changed = priorLocalMember !== member && true ;
changed = true; | ||
} | ||
} else { | ||
changed = true; |
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.
Even this if conditions should be optimised as like suggested above
@@ -135,62 +132,3 @@ module('integration/application - Using the store as a service', function (hooks | |||
assert.notStrictEqual(store, secondService, 'the store can be used as a service'); | |||
}); | |||
}); | |||
|
|||
module('integration/application - Attaching initializer', function (hooks) { |
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.
Are these test changes intentional?
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 tests test a removed thing, by testing the removed thing they cause a deprecation because its not really there. They were unrelated but I was tired of the noise when running the test suite
imrpove handling of rollback scenarios
imrpove handling of rollback scenarios
imrpove handling of rollback scenarios
No description provided.