Skip to content
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

refactor: make the set method more reasonable #7981

Merged
merged 2 commits into from
Oct 22, 2018
Merged

refactor: make the set method more reasonable #7981

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf9c20c
refactor: avoid unnecessary and costly data observations
HcySunYang Apr 8, 2018
cf67d10
refactor: make the code more readable
HcySunYang Jun 20, 2018
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
6 changes: 5 additions & 1 deletion src/core/observer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,11 @@ export function defineReactive (
set: function reactiveSetter (newVal) {
const value = getter ? getter.call(obj) : val
/* eslint-disable no-self-compare */
if (newVal === value || (newVal !== newVal && value !== value)) {
if (
(getter && !setter) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Then when have the chance to invoke customSetter?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsonet

The following content is my understanding, there may be incorrect place.

The first thing we can guarantee is that if the value of the property property is undefined, both the value of getter and setter will be undefined:

/* defineReactive */
const property = Object.getOwnPropertyDescriptor(obj, key)
// If `property === undefined`, both `setter` and `getter` are undefined
const getter = property && property.get
const setter = property && property.set

The customSetter method is mainly used to print prompt information when the property value is set, and mainly used in the following aspects:

  • Define the injected value on the instance object:
/* inject.js  initInjections */
defineReactive(vm, key, result[key], customSetter)

At this point, the return value of Object.getOwnPropertyDescriptor(vm, key) is undefined, so both getter and setter are undefined

  • Define the $attrs and $listeners properties on the instance object:
/* render.js  initRender */
    defineReactive(vm, '$attrs', parentData && parentData.attrs || emptyObject, () => {
      !isUpdatingChildComponent && warn(`$attrs is readonly.`, vm)
    }, true)
    defineReactive(vm, '$listeners', options._parentListeners || emptyObject, () => {
      !isUpdatingChildComponent && warn(`$listeners is readonly.`, vm)
    }, true)

We can still guarantee that the following statement's value is undefined:

Object.getOwnPropertyDescriptor(vm, '$attrs') // undefined
Object.getOwnPropertyDescriptor(vm, '$listeners') // undefined
  • Define the props data on the vm._props object:
/* state  initProps */
const props = vm._props = {}
defineReactive(props, key, value, customSetter)

The value of Object.getOwnPropertyDescriptor(props, key) is undefined。

In summary, when the value of the Object.getOwnPropertyDescriptor method is undefined, both the getter and the setter are undefined, so (getter && !setter) will always be false and will not affect the execution of the customSetter.

Copy link
Member

Choose a reason for hiding this comment

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

But won't it make the code harder to maintain? Since every time you add a call to defineReactive with customSetter you'll have to check all these conditions.

IMHO, as this refactor aims to get rid of unnecessary observation, putting the getter and setter check right before assigning to val might be easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right

Copy link
Member Author

Choose a reason for hiding this comment

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

@sodatea I have made some changes.

newVal === value ||
(newVal !== newVal && value !== value)
) {
return
}
/* eslint-enable no-self-compare */
Expand Down