-
Notifications
You must be signed in to change notification settings - Fork 46.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
Remove unnecessary check #7141
Remove unnecessary check #7141
Conversation
This seems faster, because you are not assigning a variable, then doing a check, then possibly assigning another variable. You're just assigning one variable.
It seems faster in code but it adds N-1 DOM operations. Speculatively, that will be more expensive (though will be browser dependent, but even setting a property to the same value likely needs to cross the bridge from JS to C++). I'm going to say that we should just leave this as is. Thanks though! |
Cool, thanks for the info |
@zpao I'm not sure I buy that argument. Any browser that had the value available in JS (ie. available for read) would probably check that value for a change before passing it into C++. I think most browsers do have the value available in JS and do check before passing the write to C++ (the exception being moderately old versions of IE). Anyway, I give it a 51% chance that @zwhitchcox's code is actually better/faster for all the cases we care about. @zwhitchcox I'd be interested in seeing a benchmark. If it is faster, I think it would be reasonable to take this change. |
cc @sebmarkbage (he has a dream of always doing writes) |
Write all the things! The only case this would not be faster is when we can compare to a JS model and that is local. This is comparing to a DOM object. So yea, I agree with @jimfb and OP. Sometimes, a setter will cause other side-effects though so we have to be careful with that. This seems fine though. |
function updateOptions(inst, multiple, propValue) {
propValue = [].concat(propValue) // makes propValue an array even if it's just a single value
ReactDOMComponentTree
.getNodeFromInstance(inst)
.options
.forEach(function(opt) {
(propValue.indexOf(opt.value) < 0 ? opt.selected : !opt.selected) &&
opt.selected = !opt.selected
})
} |
Nevermind...I broke things |
This seems faster, because you are not assigning a variable, then doing a check, then possibly assigning another variable. You're just assigning one variable.