Skip to content

perf: shrink instrumentations requires toRaw parameter #9051

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

perf: shrink instrumentations requires toRaw parameter #9051

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/reactivity/src/baseHandlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function createArrayInstrumentations() {
const res = arr[key](...args)
if (res === -1 || res === false) {
// if that didn't work, run it again using raw values.
return arr[key](...args.map(toRaw))
return arr[key](toRaw(args[0]), ...args.slice(1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary, because the parameters passed in by the user are uncontrollable. The parameters passed in by the user may all be responsive objects. Only use toRaw to process the first parameter. If the subsequent parameters are responsive objects, Incur access tracking overhead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary, because the parameters passed in by the user are uncontrollable. The parameters passed in by the user may all be responsive objects. Only use toRaw to process the first parameter. If the subsequent parameters are responsive objects, Incur access tracking overhead

The purpose here is to use the original value to re-check whether it exists, so only the first parameter needs to be toRaw, and other parameters do not require additional processing. At the same time, the modification here will not cause additional overhead, because before executing this code, it will execute const res = arr[key](...args)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great if we could add or point out a test case to clarify it further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great if we could add or point out a test case to clarify it further.

// reactiveArray.spec.ts
test('Array identity methods should work with raw values', () => {
    const raw = {}
    const arr = reactive([{}, {}])
    arr.push(raw)
    expect(arr.indexOf(raw)).toBe(2)
    expect(arr.indexOf(raw, 3)).toBe(-1)

    // should work also for the observed version
    const observed = arr[2]
    expect(arr.indexOf(observed)).toBe(2)
    expect(arr.indexOf(observed, 3)).toBe(-1)
  })

if the code is

arr.indexOf(observed, 3, anotherObserved, anotherObserved, anotherObserved, anotherObserved, anotherObserved)

we just need to use toRaw for the first param, and we dont need to use toRaw for anotherObserved.

} else {
return res
}
Expand Down