Updates for performance and some bug fixes #1238
Conversation
atkulp
commented
Dec 5, 2022
•
atkulp
commented
| Q | A |
|---|---|
| Is bugfix? | ✔️ |
| New feature? | ✔️ |
| Is backward-compatible? | ✔️ |
| Tests pass? | ✔️ |
| Fixed issues | general fixes and enhancements (covered in issue #1239) |
| Updated README/docs? | ❌ |
| Added CHANGELOG entry? | ✔️ |
src/utilities.js
Outdated
| @@ -25,6 +25,14 @@ export function deepCopy (target) { | |||
| return isPlainObject(target) ? extend({}, target) : Array.isArray(target) ? target.map(deepCopy) : target | |||
| } | |||
|
|
|||
| export function extend2 (destination, ...args) { | |||
There was a problem hiding this comment.
I did some benchmarking here a few ways. I was pretty shocked to see the original extend method here is faster than structuredClone and I also tried against JSON stringify/parse. Original did 1,911 ops/sec, structuredClone did 1,568 ops/sec (18% slower), and JSON was only 1,027 ops/sec (46% slower).
Always benchmark! I'm pretty shocked by the difference. I'll back out this
| @@ -68,6 +76,10 @@ export function hasOwnProperty (obj, key) { | |||
| return obj && Object.prototype.hasOwnProperty.call(obj, key) | |||
| } | |||
|
|
|||
| export function isNumber2 (value) { | |||
There was a problem hiding this comment.
unlike the object copying, the updated isNumber and isInteger implementation continue to be faster
|
Tests are failing. |
|
I don't understand these failures. everything succeeds locally. any ideas? |
|
I hadn't actually run the codecept tests before. now I have and it looks like the same ones fail in master. am I missing something? for example, in core_test.js, in |
|
Why is the diff so huge? CC: @germanbisurgi |
|
@atkulp Several tests are failing... |
atkulp
force-pushed
the
array-defer-plus
branch
from
8b28283
to
bc0e740
Compare
|
ok. I hope this doesn't make things any worse. I found some bad stuff in my commits so I fixed it up and force pushed it back. if you'd rather I abandon this and do it on a new branch, I understand. it seems to be passing tests and building properly now |
|
I've started fixing the failing tests locally, but they all seem to be based on errors in the codeceptjs scripts (or maybe some breaking changes?). I can't see how my changes would possibly affect it as they are things like multiple elements with the same selector (.alert-danger) returning the first value so failing the test. I can continue and check in my fixes, but I wanted to make sure there wasn't something else at play
|
|
@germanbisurgi Could you have a look, please. |
There was a problem hiding this comment.
Hi @atkulp, thanks for your pull request. However, we've found that some of the changes may break important features. For example, the core validation changes produce undefined errors when initializing the editor, and setting the value of an instance containing opt_in fields didn't work when testing.
To address these issues, we kindly ask you to split your pull request into smaller ones that focus on specific changes. This will make it easier for us to review and test each change independently, and reduce the risk of breaking existing features.
We also recommend creating an issue page that describes the current behavior, the expected behavior, and the proposed changes for each pull request. This will provide clear context for each change and help reviewers and contributors understand the reasons behind the proposed changes.
Thank you for your contribution, and please let us know if you have any questions or concerns.
