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.

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(effect): use includes instead of indexOf #286

Merged
merged 1 commit into from
Oct 15, 2019
Merged

refactor(effect): use includes instead of indexOf #286

merged 1 commit into from
Oct 15, 2019

Conversation

yeyan1996
Copy link
Contributor

No description provided.

@@ -88,7 +88,7 @@ function run(effect: ReactiveEffect, fn: Function, args: any[]): any {
if (!effect.active) {
return fn(...args)
}
if (activeReactiveEffectStack.indexOf(effect) === -1) {
if (!activeReactiveEffectStack.includes(effect)) {
Copy link
Member

Choose a reason for hiding this comment

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

According to existing benchmarks, in latest versions of Chrome these two approaches are pretty close on performance. Shall we prefer includes here? If we never use includes in our code base we potentially reduces the need to polyfill Array.prototype.includes for future compatible builds (thus may result in smaller size overhead) but it migh be too trivial. WDYT? @yyx990803

Copy link
Contributor Author

Choose a reason for hiding this comment

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

emmm,I don't know the performance difference between indexOf and includes,but there are other places used includes

// src/scheduler.ts
export function queueJob(job: () => void) {
  if (!queue.includes(job)) {
    queue.push(job)
    if (!isFlushing) {
      nextTick(flushJobs)
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

My point is if we prefer indexOf over includes for polyfill reasons we should eliminate all usage of includes in our codebase. Otherwise we should always prefer includes because performance is pretty close.

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's use includes everywhere since it's more readable and semantically correct. The polyfill is pretty light too. The general idea is we will use modern features as long as it's an objective improvement over ES5 counterparts and not too costly to polyfill in IE11.

@yyx990803 yyx990803 merged commit 9a37c4b into vuejs:master Oct 15, 2019
@vue-bot
Copy link
Contributor

vue-bot commented Oct 15, 2019

Hey @yeyan1996, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants