Use faster implementation for merging #186
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
melvin-bot
bot
requested review from
AndrewGable
and removed request for
a team
|
@iwiznia Could you take a look as you recently optimised the same part? |
There was a problem hiding this comment.
@Szymon20000 You will also need to sign the CLA in this repo as well!
Regarding the JSDoc, lets update the indentation as you can see here, for example https://github.com/Expensify/App/blob/da6e3aac5b64154e1d87c261f34829e097c7b433/src/libs/ReportUtils.js#L388-L393
ie the next line has one whitespace at the start of the line. Thanks!
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
@Szymon20000 I am also thinking if we should update the Docs for the functions, they always just copy what the function name is so it is not very useful. So we should either expand on it (why it is there/what it is used for - which would be handy imho) or remove them as they dont add any value as its current form
There was a problem hiding this comment.
The code looks good, but I think the ESlint warnings need to be cleaned up better. The only time an eslint rule should be disabled is when there is a business reason for it (like performance). Otherwise, you should try to follow them rather than disable them.
| destination[key] = cloneIfNecessary(source[key], optionsArgument) | ||
| } else { | ||
| destination[key] = merge(target[key], source[key], optionsArgument) | ||
| // eslint-disable-next-line rulesdir/prefer-underscore-method |
There was a problem hiding this comment.
Is there any reason not to use underscore here? If it's because of performance, please add a code comment explaining that.
There was a problem hiding this comment.
Why does the function has to be defined as mergeObject = function (target, source) instead of simply function mergeObject(target, source)?
There was a problem hiding this comment.
We have to do that otherwise we would use it before it's defined, since we have the following circular dependency mergeObject -> fastMerge -> mergeObject
There was a problem hiding this comment.
when 2 functions are on the same level, they are registered at the same time, it's just that eslint would flag it as a code style issue, because our code style is to define stuff before it's used - impossible to address in case of a circular dependency, but we can just disable the warning
The actual problem is with variables - you can't use a variable before it is defined, but you can use a function before it's defined, because functions are always registered before any code runs
There was a problem hiding this comment.
Thanks for clarifying @kidroca. In that case, I agree that we should declare it as function mergeObject() and disable the lint warning with a comment explaining why
| destination[key] = cloneIfNecessary(source[key], optionsArgument) | ||
| } else { | ||
| destination[key] = merge(target[key], source[key], optionsArgument) | ||
| // eslint-disable-next-line rulesdir/prefer-underscore-method |
There was a problem hiding this comment.
Sorry to keep harping on the style, but we are sticklers for consistency. Please try to follow previous patterns and our style guides https://github.com/Expensify/Style-Guides/blob/main/javascript.md
lib/fastMerge.js
Outdated
| /** | ||
| * Is the object Mergeable | ||
| * | ||
| * @param {*} val | ||
| * @returns {*|boolean} | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Is the object Mergeable | |
| * | |
| * @param {*} val | |
| * @returns {*|boolean} | |
| */ | |
| /** | |
| * @param {mixed} val | |
| * @returns {mixed} | |
| */ |
Don't include a method description unless it's valuable (and what you had there just repeats the method name). For the return value, maybe we should cast it to always return a boolean?
Co-authored-by: Tim Golen <tgolen@gmail.com>
The whole problem is described here Expensify/App#11185
On Android merging takes a lot of time
I Initially thought that maybe it's caused by slow loads implementation but after looking more deeply into the problem it looks like require takes in lodash a lot of time. Have no idea how because it should be required only once and later not take time at all but it's not the case. Maybe some circular dependencies?
I used the latest react-native-onyx package .18 with recent fixes like getting rid of merging items of array one by one. But it seems to no solve the problem.
Details
Related Issues
GH_LINK
Automated Tests
Linked PRs